-
Notifications
You must be signed in to change notification settings - Fork 3k
Add RFC for PinMap's extension #8693
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
Changes from all commits
25ac7d2
1dec293
e21407c
84de41f
261f74c
6be8b54
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,107 @@ | ||
# HAL PinMap | ||
|
||
<!-- toc --> | ||
|
||
- [Description](#Description) | ||
- [Motivation](#Motivation) | ||
- [Requirements](#Requirements) | ||
- [Design choices](#Design-choices) | ||
- [Questions](#Questions) | ||
|
||
<!-- tocstop --> | ||
|
||
## Description | ||
|
||
Adds board introspection capabilities and tools. | ||
These tools will help designing better tests for the HAL. | ||
|
||
## Motivation | ||
|
||
At the time being, drivers are only tested on a single "default" peripheral. However, some target feature the same peripheral through different blocks' implementations for example the SPI may be provided on a single MCU by its USART, QSPI and SSP peripherals. | ||
To ensure that the driver's implementation is valid for all these peripherals we want the CI to run the test set on each peripheral using at least one set of pin determined at run time (pin may eventually picked randomly). | ||
|
||
## Requirements | ||
|
||
1. We want to list all pins for a function on a peripheral. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is eg all SPI MOSI pins? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is all SPI MOSI for SPI1. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be worth saying that in the doc ("for instance, all pins that can be configured as MOSI for SPI1") |
||
For instance, all pins that can be configured as MOSI for SPI1. | ||
2. We want to list all functions a pin can provide. | ||
3. We want to list all pins for a form-factor regardless of the function they can provide. | ||
4. We want a printable name for each pin and a method to get that name. | ||
5. We want a list of the reserved pins that cannot be tested. | ||
6. We want to select a duplicate-free set a pin from a set of PinMap. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a bit unclear for me. Do we mean selecting a subset of the given PinMap? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let us consider this example : PinMap *maps[] = {
spi_get_mosi_pins(false),
spi_get_miso_pins(false),
spi_get_clk_pins(),
spi_get_cs_pins()
};
PinName pins[4];
pinmap_find_peripheral_pins(maps,
pinmap_form_factor_pins(PINMAP_FORM_FACTOR_ARDUINO_ZERO),
pinmap_restricted_pins(),
SPI2,
pins,
4
);
// pins would be :
// pins = { a_pin_from_mosi_pins,
// a_pin_from_miso_pins,
// a_pin_from_clk_pins,
// a_pin_from_cs_pins
// };
//
// None of them being equal. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All clear now. 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't get that either, I'm assuming you mean "We want to select a duplicate-free set of pins from a set of PinMaps." But I still don't see what that means. Looking at the example doesn't make it clearer and the function is not defined below (unless it's the same as _pin below). What is that for? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The I edited the previous answer to match new most recent PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the use case there? Why do we need that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is what the tests will use to pick the pins from the lists. |
||
|
||
Any of the list mentioned above may be empty if none match the criteria. | ||
|
||
## Design choices | ||
|
||
These requirements do not impact the current API and thus they can all be implemented as an extension to the existing API. | ||
It is proposed to introduce the following elements : | ||
- `DEVICE_PIN_EXTENDED_SUPPORT` A `device_has` flag indicating the implementation of this extension. | ||
- The following part of the API provides the features needed for requirements 3, 4 and 5 | ||
```c | ||
enum pinmap_form_factor_ { | ||
PINMAP_FORM_FACTOR_ARDUINO_ZERO, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is that growable? If yes, what's the impact? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the impact is null as pinmap_form_factor_pins return NULL for unsupported/unknown form factors. |
||
PINMAP_FORM_FACTOR_ARDUINO_DUE, | ||
PINMAP_FORM_FACTOR_NXP_FRDM, | ||
PINMAP_FORM_FACTOR_ST_MORPHO, | ||
PINMAP_FORM_FACTOR_MTB, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should clarify how a partner would contribute to that - also, if you go down to the chip/module level - that's almost one form factor (sometimes more!) per chip/module 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would most probably require the definition of an array per form factor defining which pin are available in this FF. |
||
} pinmap_form_factor_t; | ||
|
||
/// returns a static NC terminated array of pins (or NULL if unsupported). | ||
const PinName *pinmap_form_factor_pins(pinmap_form_factor_t form_factor); | ||
/// returns a static NC terminated array of pins. | ||
const PinName *pinmap_restricted_pins(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is probably an implementation detail -- should we provide a Or are these supposed to be static, like other arrays fomr There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are indeed supposed to be static. |
||
/// returns a null (\0) terminated static character array representing the name of the pin. | ||
const char *pinmap_pin_to_string(PinName pin); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any naming rules? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about using the vendor's naming convention ? |
||
``` | ||
- These generic function make use of the existing API to answer requirements 1, 2 and 6. | ||
```c | ||
/// Returns the `n`'th pin supporting the `peripheral` in the `map`. | ||
PinName pinmap_find_peripheral_pin(const PinMap *map, int peripheral, uint32_t n); | ||
|
||
/// Returns a pin from ((whitelist ∩ map) − blacklist). | ||
/// Typically, whitelist will be the form factor list of pins. | ||
/// The blacklist may be a list of pins already visited and/or tied to another peripheral. | ||
PinName pinmap_find_peripheral_pin(const PinMap *map, const PinName *whitelist, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I hope that's a helper function that works on "sets" rather than something partners will need to do. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed these functions are not platform dependant. |
||
const PinName *blacklist, int per); | ||
|
||
/// Verifies that `pin` is in `list`. | ||
bool pinlist_has_pin(const PinName *list, PinName pin); | ||
|
||
/// Applies pinmap_find_peripheral_pin to each map in maps ensuring a pin will not be used twice. | ||
/// @param[in] maps A list of pinmap. | ||
/// @param[in] count Number of elements of maps and pins. | ||
/// @param[in] whitelist An NC terminated list of pins. | ||
/// @param[in] blacklist An NC terminated list of pins. | ||
/// @param[in] per A peripheral name. | ||
/// @param[out] pins An array of PinName where the result is stored. | ||
bool pinmap_find_peripheral_pins(const PinMap const **maps, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need pin and pins version of this function? How will you know how big is the out array? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the _pin only work for a single pinmap and returns a single pin. These two functions are generic and are not implemented by partners. |
||
uint32_t count, | ||
const PinName *whitelist, | ||
const PinName *blacklist, | ||
uint32_t per, | ||
PinName *pins); | ||
``` | ||
- Additionally to these requirements any peripheral API must expose adequate functions to fetch an NC terminated list of pins associated to each of their their functions. These API extensions shall be gated with a `<PERIPHERAL_NAME>_PIN_EXTENTION` where `<PERIPHERAL_NAME>` is the name of the considered peripheral (`SPI`, `I2C`, `CAN` etc). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need these specific wrappers? If they are not required by the solution why are we mentioning them? That not only adds unnecessary functions, but also pollutes "device_has" even more than it is already. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some device may require specific implementation of the wrapper. A platform without this requirement would use the default implementation that returns SPI_PinMap_MISO. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't we just treat SPI Master and SPI Salve as two specific features? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would need to be discussed in the appropriate PR when these methods will be introduced. |
||
|
||
For example the SPI api may be extended with : | ||
```c | ||
#ifdef DEVICE_SPI_PIN_EXTENDED_SUPPORT | ||
PinMap *spi_get_mosi_pins(bool as_slave, bool is_half_duplex); | ||
PinMap *spi_get_miso_pins(bool as_slave, bool is_half_duplex); | ||
PinMap *spi_get_clk_pins(); | ||
PinMap *spi_get_cs_pins(); | ||
#endif | ||
``` | ||
|
||
and the I²C api with : | ||
```c | ||
#ifdef DEVICE_I2C_PIN_EXTENDED_SUPPORT | ||
PinMap *i2c_get_scl_pins(); | ||
PinMap *i2c_get_sda_pins(); | ||
#endif | ||
``` | ||
|
||
|
||
## Questions | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this paragraph needs expanding a little bit