Update template argument names to not collide with Arduino names

[kyle-cypress]

Description

Several templated classes use "A1", "A2", etc as type argument names, which are the same as what mbed requires for Arduino header pin names, which can lead to naming conflicts when board targets define these names as macros.

We noticed this issue because the Cypress board-specific targets provide a header with macros which map arduino pin names to elements in a chip-specific enum. E.g.

#define A1 P10_0

If parentheses are added around the macro value (per best practice), compilation fails with various, highly verbose errors related to the FunctionPointer template. Without the parentheses (what is in mbed today) the build succeeds, presumably because the #defines simply act as a still-valid rename of the template parameters.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

Release Notes

[ciarmcom]

@kyle-cypress, thank you for your changes.
@ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

[0xc0170]

I am not completely certain this is way to go. What is the use of #define A1 P10_0 ?

If you need a macro magic world, you can do in pinname header file:
A1 = CYPRESS_ARDUINO_A1

CYPRESS_ARDUINO_A1 can be a macro to get a proper pinout. That way you won't have any conflicts.

I would not expect having A1 as macro - very short, not enough descriptive - leads to problems like this one

[kjbracey]

I think my counter-suggestion to the issue is "don't do that then". I guess this is a reason why A1 etc are normally enum values. (Although that does mean we can't test for them with #ifdef - you have to have other macros to indicate availability like TARGET_FF_ARDUINO).

But anyway, it appears as long as A1 is a simple single-token identifier define, you get away with it in the Mbed OS build - you certainly do in this case, as P10_0 is a valid identifier.

There's certainly no need to add any parentheses to #define A1 P10_0, as it's just defined to a single token. That could only ever fail as a macro if P10_0 itself was a broken macro, and you were covering for it. Parentheses around macro definitions are only required if it's a multi-token expression.

CYPRESS_ARDUINO_A1 can be a macro to get a proper pinout. That way you won't have any conflicts.

If you are an Arduino form-factor board (TARGET_FF_ARDUINO), then you are required to provide definitions for A1 etc. But I've only ever seen them as enum values for PinNames up to now.

[0xc0170]

If you are an Arduino form-factor board (TARGET_FF_ARDUINO), then you are required to provide definitions for A1 etc. But I've only ever seen them as enum values for PinNames up to now.

Yep, it should be PinNames. Means this PR should not be needed.

[kjbracey]

Means this PR should not be needed.

Further than that, an alternative corrective PR to make those defines into enums doesn't seem to be that urgent, as the boards in question have been getting away with the define for a while - it's breaking down if the define is changed to (P10_0) with unnecessary parentheses.

[kyle-cypress]

The motivation for having the A1 etc aliases as #defines is that it makes for a cleaner target architecture:

• A "Chip Support" layer understands the underlying MCU parts around which the boards are built. This layer provides the PinNames enum (because it knows all of the pins that exist on the chip). But it can't provide names like A1 because those are a function of the board, not the chip (the same chip could be placed on multiple boards, each of which might map the A1 header connection to different pins, or which might not provide an arduino header at all). So the PinNames enum only contains entries for the Cypress-defined pin identifiers (which are of the form Pn_m).
• A "Board Support" layer builds on top of of the "Chip Support" layer to provide board-specific knowledge. In particular, it provides a mapping from standard function names (e.g. arduino headers, debug uart pins) to the pins that the board connects for those functions. This is implemented by providing the names as macros which resolve to entries in the Chip Support-defined PinNames enum.

This avoids duplication between boards which use the same underlying MCU.

The macro-aliasing approach seemed the cleanest way to achieve this, at least until we encountered this issue. If what we're doing is not common/expected/supported, we can pursue alternative approaches (as a side note: if it's not there already, can the porting guide add a note about this usage being unsupported?).

For reference, here are some of the options we considered and rejected. I make no claim that this is an exhaustive list of all possibilities; we thought the macro approach was suitable and did not continue evaluating.

• Defining a second enum in the board support layer for the board-defined pin aliases can produce warnings about implicitly converting between enum types.
• Providing a #include-style mechanism for the boards to inject entries into the PinNames enum definition introduces a circular dependency between the chip support and board support layers, when the former should know nothing about the latter.
• Moving the PinNames enum into the board support layer wholesale introduces significant duplication between boards built around the same MCU (which is a common occurrence in the Cypress targets)

[kjbracey]

Providing a #include-style mechanism for the boards to inject entries into the PinNames enum definition introduces a circular dependency between the chip support and board support layers, when the former should know nothing about the latter.

Can I suggest the opposite?

// Board PinNames.h
enum PinNames {
#include "MCUPinNameEnums.h"
    A1 = P10_0,
    ...
};

[0xc0170]

This is similar to what I had on my mind ^^ we should be able to go with one-way inclusion.

[0xc0170]

@kyle-cypress Any update?

[kyle-cypress]

We discussed the suggestion to invert the inclusion order and it seems like it would still create a dependency from the Chip support (CSP) on the Board support (BSP).
Many of the CY HAL APIs (which belong at the chip support level) require a PinName enum member as an argument. So either the CSP source files would need to depend on a header from the BSP in order to provide the actual enum declaration, or (if the CSP APIs used a different PinName type) every call using the BSP PinNames type would need to cast it to the CSP PinNames type.
However, we have identified a couple of other possible solutions which are currently being evaluated for feasibility. I will update this ticket with our results.

[0xc0170]

@kyle-cypress Please reopen once updated. I'll close this for the meantime as it's not progressed for a month.