Skip to content

BlueNRG-MS: Use non Arduino Uno pin names #14829

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

LDong-Arm
Copy link
Contributor

Summary of changes

The Arduino Uno pin aliases in hal/PinNameAliases.h are now guarded by the macro TARGET_FF_ARDUINO_UNO (since 7c333d8) and not globally available anymore. This results in the following error (and similar ones) when building the driver for K64F:

error: 'ARDUINO_UNO_SPI_MOSI' was not declared in this scope

To fix it, replace the aliases with their raw pin names. Now the pins are identical to BlueNRG-2.

Impact of changes

Migration actions required

Documentation

None


Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[x] Tests / results supplied as part of this PR

Manual testing: compile mbed-os-example-ble/BLE_Advertising for K64F.


Reviewers

@ARMmbed/mbed-os-connectivity


The Arduino Uno pin aliases in `hal/PinNameAliases.h` are now
guarded by the macro `TARGET_FF_ARDUINO_UNO` (since 7c333d8) and
not globally available anymore. This results in the following
error (and similar ones) when building the driver for K64F:

    error: 'ARDUINO_UNO_SPI_MOSI' was not declared in this scope

To fix it, replace the aliases with their raw pin names. Now the
pins are identical to BlueNRG-2.
@0xc0170 0xc0170 requested a review from a team June 24, 2021 15:17
Copy link
Collaborator

@jeromecoutant jeromecoutant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should correct K64F...

@MarceloSalazar

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 24, 2021

Why then BlueNRG2 uses D/A pin names? What is the difference ?

    "name": "bluenrg_2",
    "config": {
        "SPI_MOSI":  "D11",
        "SPI_MISO":  "D12",
        "SPI_nCS":   "A1",
        "SPI_RESET": "D7",
        "SPI_IRQ":   "A0",
        "SPI_SCK":   "D3",
        "valid-public-bd-address": {
            "help": "Read the BD public address at startup",
            "value": false
        }
    },

The Arduino Uno pin aliases in hal/PinNameAliases.h are now guarded by the macro TARGET_FF_ARDUINO_UNO (since 7c333d8) and not globally available.

Were they available for K64F? I don't see ARDUINO_UNO supported by K64F, only ARDUINO .

@LDong-Arm
Copy link
Contributor Author

I think you should correct K64F...

@MarceloSalazar

@jeromecoutant Do you mean use Arduino pins by default, and override K64F to not use it? Or enable ARDUINO_UNO for K64F?

@ARMmbed/mbed-os-hal We have two form factors in targets.json - ARDUINO and ARDUINO_UNO. Arduino has the former, thus Uno pins are not available. Are they the same?

@0xc0170 0xc0170 self-requested a review June 24, 2021 15:28
"SPI_RESET": "ARDUINO_UNO_D7",
"SPI_IRQ": "ARDUINO_UNO_A0",
"SPI_SCK": "ARDUINO_UNO_D3",
"SPI_MOSI": "D11",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, hardcoding this to D11, how to overwrite it as this is acomponent to be used by other targets. It was chosen to be usable with arduino uno. A question is, shouldn't this be a specific config and available to be overriden by a target?

"SPI_MOSI": "MBED_CONF_BLUENRG_MOSI", etc ?

Copy link
Contributor Author

@LDong-Arm LDong-Arm Jun 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mbed_lib.json is the very place to generate MBED_CONF_..., so we shouldn't reference MBED_CONF_... in mbed_lib.json. The field target_overrides is the exact place for overriding pins per target.

In my opinion, providing defaults still makes sense. This is convenient for targets supporting a common form factor (e.g. ARDUINO_UNO) so they don't need any overrides.

@LDong-Arm
Copy link
Contributor Author

Why then BlueNRG2 uses D/A pin names? What is the difference ?

This is a question for @apalmieriGH and @ARMmbed/mbed-os-connectivity who added BlueNRG 2 support.

Were they available for K64F? I don't see ARDUINO_UNO supported by K64F, only ARDUINO .

As per my inline comment, the Uno pins were available (probably by mistake to both ARDUINO_UNO and ARDUINO form factors, before 7c333d8.

@MarceloSalazar
Copy link

K64F needs to migrate to the standard Arduino Uno pin names - see docs

ARDUINO is the legacy (deprecated). ARDUINO_UNO is the supported one.

@LDong-Arm
Copy link
Contributor Author

LDong-Arm commented Jun 24, 2021

K64F needs to migrate to the standard Arduino Uno pin names - see docs

ARDUINO is the legacy (deprecated). ARDUINO_UNO is the supported one.

Thanks for clarification. I think the fix would then be migrating K64F to ARDUINO_UNO. Is that easy to do?

I'm closing this PR, but until it's fixed this issue blocks Travis CI of mbed-tools.
Additionally, it would be good to switch to ARDUINO_UNO pins for BlueNRG-2 too.

@LDong-Arm LDong-Arm closed this Jun 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants