Skip to content

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

Closed
wants to merge 6 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 107 additions & 0 deletions docs/design-documents/hal/0002-pinmap-extension.md
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.
Copy link
Contributor

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


## 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.
Copy link
Member

Choose a reason for hiding this comment

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

That is eg all SPI MOSI pins?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is all SPI MOSI for SPI1.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

@ithinuel ithinuel Nov 15, 2018

Choose a reason for hiding this comment

The 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.

Copy link
Member

@fkjagodzinski fkjagodzinski Nov 15, 2018

Choose a reason for hiding this comment

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

All clear now. 👍

Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

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

The _bus was used before an update that came on filip's review.

I edited the previous answer to match new most recent PR.
Let me know if you stil have questions.

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the use case there? Why do we need that?

Copy link
Member Author

Choose a reason for hiding this comment

The 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,
Copy link
Member

Choose a reason for hiding this comment

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

Is that growable? If yes, what's the impact?

Copy link
Member Author

Choose a reason for hiding this comment

The 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,
Copy link
Contributor

Choose a reason for hiding this comment

The 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 😅

Copy link
Member Author

Choose a reason for hiding this comment

The 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.
What do you think would be the more appropriate ?

} 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();
Copy link
Member

Choose a reason for hiding this comment

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

This is probably an implementation detail -- should we provide a pinmap_free() function or tell the user to use the generic one?

Or are these supposed to be static, like other arrays fomr PeripheralPins.h?

Copy link
Member Author

Choose a reason for hiding this comment

The 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Any naming rules?

Copy link
Member Author

Choose a reason for hiding this comment

The 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,
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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,
Copy link
Member

Choose a reason for hiding this comment

The 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? Number of elements of maps and pins what does it mean? Is maps and pins parameters need to be the same length?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.
In _pins :
maps & pins have the same length/number of elements.
pinmap_find_peripheral_pin is called for each map in maps including the previously returned pin to the black list.
pinmap_find_peripheral_pins returns a valid combination of pins ensuring that a pin does not appear more than once in pins.

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).
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Some device may require specific implementation of the wrapper.
Eg: K66F platform do not have a defined list of MOSI pins and MISO pins that work for both MASTER & SLAVE mode.
It has instead a SOUT and a SIN list of pins that are always respectively output or input.
a spi_get_miso_pins(bool is_slave); would select which pin list to return according to the is_slave argument.

A platform without this requirement would use the default implementation that returns SPI_PinMap_MISO.

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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