Add RFC for PinMap's extension #8693

ithinuel · 2018-11-09T15:41:20Z

Description

This proposal adds an extension to PinMap's API.
This will allow advanced test to arbitrarily select a peripheral and configure the pins to test.

Pull request type

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

adbridge · 2018-11-14T15:05:16Z

@ARMmbed/mbed-os-hal @c1728p9 This has been awaiting review for 5 days now , could you please ensure it is looked at asap.

fkjagodzinski · 2018-11-15T10:55:17Z

docs/design-documents/hal/0002-pinmap-extension.md

+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 the reserved pins that cannot be tested.
+6. We want to select a duplicate-free set a pin from a set of PinMap.


This is a bit unclear for me. Do we mean selecting a subset of the given PinMap?

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.

All clear now. 👍

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?

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.

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

This is what the tests will use to pick the pins from the lists.

fkjagodzinski · 2018-11-15T10:59:58Z

docs/design-documents/hal/0002-pinmap-extension.md

+  /// returns an NC terminated array of pins.
+  const PinName *pinmap_form_factor_pins(pinmap_form_factor_t form_factor);
+  /// returns an NC terminated array of pins.
+  const PinName *pinmap_restricted_pins();


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?

These are indeed supposed to be static.

fkjagodzinski · 2018-11-15T11:53:08Z

docs/design-documents/hal/0002-pinmap-extension.md

+  /// 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_pins(const PinMap *map, const PinList *whitelist,


Since this fun returns a single PinName I think the plural _pins part of the name could confuse some users. Also do we need both whitelist and blacklist?

Indeed, I fixed the name of the function.

The ((whitelist ∩ map) − blacklist) operation could be achieve before calling this function but it might be easier to use if it is done internally. you wouldn't have to reimplement it for each tests.

fkjagodzinski

No more questions from me. Has anyone from @ARMmbed/mbed-os-hal anything to add?

bulislaw

I'm missing description of how it is defined in target and what partners are expected to give us and what is "platform" code working on that input.

bulislaw · 2018-11-22T15:53:43Z

docs/design-documents/hal/0002-pinmap-extension.md

+
+## Description
+
+This update aims at increasing tests thoroughness by enabling them to check driver's reliability for each pin declared as supporting a peripheral.


This sentence is killing me, can we rewrite it in readable way.

bulislaw · 2018-11-22T15:55:04Z

docs/design-documents/hal/0002-pinmap-extension.md

+
+## Requirements
+
+1. We want to list all pins for a function on a peripheral.


That is eg all SPI MOSI pins?

That is all SPI MOSI for SPI1.

Would be worth saying that in the doc ("for instance, all pins that can be configured as MOSI for SPI1")

bulislaw · 2018-11-22T15:56:33Z

docs/design-documents/hal/0002-pinmap-extension.md

+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 the reserved pins that cannot be tested.


bulislaw · 2018-11-22T16:04:28Z

docs/design-documents/hal/0002-pinmap-extension.md

+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 the reserved pins that cannot be tested.
+6. We want to select a duplicate-free set a pin from a set of PinMap.


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?

bulislaw · 2018-11-22T16:05:10Z

docs/design-documents/hal/0002-pinmap-extension.md

+- 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,


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

Yes, the impact is null as pinmap_form_factor_pins return NULL for unsupported/unknown form factors.

bulislaw · 2018-11-22T16:10:06Z

docs/design-documents/hal/0002-pinmap-extension.md

+  /// 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,


I hope that's a helper function that works on "sets" rather than something partners will need to do.

Indeed these functions are not platform dependant.

bulislaw · 2018-11-22T16:12:26Z

docs/design-documents/hal/0002-pinmap-extension.md

+                                   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).


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.

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.

Can't we just treat SPI Master and SPI Salve as two specific features?

This would need to be discussed in the appropriate PR when these methods will be introduced.

bulislaw · 2018-11-22T16:15:42Z

docs/design-documents/hal/0002-pinmap-extension.md

+  /// @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,


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?

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.

donatieng · 2018-11-22T16:31:16Z

@ARMmbed/mbed-os-maintainers After a chat with @bulislaw, moving the targeted version to 5.12 as we don't need it as part of 5.11 release

bulislaw · 2018-11-22T16:57:16Z

We don't need to wait till 5.12, but it's not needed for 5.11.

cmonr · 2018-11-26T14:55:50Z

We don't need to wait till 5.12, but it's not needed for 5.11.

@bulislaw Noted.

Marking for now so that we can focus on other PRs appropriately.

donatieng

Good first draft but I'm a bit worried about the level of complexity here, both for partners and users - can we simplify a bit?

+1 on adding requirements for partners and how this API will be used.

Some MCUs have some very specific pin mapping features, for instance how would this API deal with SiLabs' crossbar feature?
https://www.silabs.com/documents/public/application-notes/AN671.pdf

donatieng · 2018-11-30T11:39:14Z

docs/design-documents/hal/0002-pinmap-extension.md

+
+## Requirements
+
+1. We want to list all pins for a function on a peripheral.


Would be worth saying that in the doc ("for instance, all pins that can be configured as MOSI for SPI1")

donatieng · 2018-11-30T11:39:48Z

docs/design-documents/hal/0002-pinmap-extension.md

+## Description
+
+Adds board introspection capabilities and tools.  
+These tools will help designing better tests for the HAL.


I think this paragraph needs expanding a little bit

donatieng · 2018-11-30T11:41:30Z

docs/design-documents/hal/0002-pinmap-extension.md

+
+## Motivation
+
+At the time being, drivers are only tested on a single "default" peripheral. However, some target features the same peripheral through different IP for example the SPI may be provided on a single MCU by its USART, QSPI and SSP peripherals. To ensure that the driver implementation is valid for all these peripherals we want the CI to assess each pin in at least one pin configuration for each peripheral.


"different IP" feels a bit vague to me - what do you think of "different block implementations" or something like that?

"some target features" -> "feature"

"we want the CI to assess each pin in at least one pin configuration for each peripheral" -> pins are not assessed there, I think features are

donatieng · 2018-11-30T11:47:35Z

docs/design-documents/hal/0002-pinmap-extension.md

+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 the reserved pins that cannot be tested.
+6. We want to select a duplicate-free set a pin from a set of PinMap.


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

donatieng · 2018-11-30T11:50:31Z

docs/design-documents/hal/0002-pinmap-extension.md

+      PINMAP_FORM_FACTOR_ARDUINO_DUE,
+      PINMAP_FORM_FACTOR_NXP_FRDM,
+      PINMAP_FORM_FACTOR_ST_MORPHO,
+      PINMAP_FORM_FACTOR_MTB,


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 😅

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 ?

donatieng · 2018-11-30T11:51:32Z

docs/design-documents/hal/0002-pinmap-extension.md

+  /// returns a static NC terminated array of pins.
+  const PinName *pinmap_restricted_pins();
+  /// returns a null (\0) terminated static character array representing the name of the pin.
+  const char *pinmap_pin_to_string(PinName pin);


Any naming rules?

What about using the vendor's naming convention ?

donatieng · 2018-11-30T11:55:36Z

docs/design-documents/hal/0002-pinmap-extension.md

+                                   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).


Can't we just treat SPI Master and SPI Salve as two specific features?

Looks like changes were addressed. Please re-review.

cmonr · 2018-12-21T03:41:28Z

@donatieng Please re-review.

0xc0170 · 2019-01-21T14:21:53Z

Shall we resume reviewing this RFC ?

c1728p9 · 2019-01-22T03:35:19Z

Pinmap code is ready for review directly in #9449. Would updating this RFC be useful to the review process or would it be easier to review the design only in #9449? If the latter then I'll close this PR.

SenRamakri

@ithinuel - The Pinmap design document looks good, but please follow the design-document template. Note that the template is flexible, so you are not required to fill all the sections if it doesnt apply.

0xc0170 · 2019-02-11T17:25:23Z

Pinmap code is ready for review directly in #9449. Would updating this RFC be useful to the review process or would it be easier to review the design only in #9449? If the latter then I'll close this PR.

What's the status? Pinmap was integrated to master, let us know how to progress here (this should have been integarted first).

c1728p9 · 2019-02-11T18:11:17Z

@0xc0170 I think we should close this. The design document should be a collaboration between architects and engineers. The review and collaboration was done as part of #9449. I don't see a benefit to updating this.

cmonr · 2019-02-11T23:50:35Z

@ithinuel Thoughts on @c1728p9 reply?

I didn't notice any comments from you in the referenced PR: #9449

c1728p9 · 2019-02-12T21:36:41Z

@0xc0170 or @cmonr the content has been updated and moved to #9690. Feel free to close this.

add the RFC for PinMap's extension

25ac7d2

cmonr added release-version: 5.11.0-rc1 needs: review labels Nov 9, 2018

cmonr requested review from a team and c1728p9 November 9, 2018 16:25

add form_factors

1dec293

fkjagodzinski reviewed Nov 15, 2018

View reviewed changes

ithinuel added 2 commits November 15, 2018 12:23

fix types names.

e21407c

add precision about static arrays and fix functions' names

84de41f

fkjagodzinski approved these changes Nov 15, 2018

View reviewed changes

0xc0170 requested review from a team and bulislaw November 20, 2018 09:23

bulislaw reviewed Nov 22, 2018

View reviewed changes

donatieng added release-version: 5.12.0-rc1 and removed release-version: 5.11.0-rc1 labels Nov 22, 2018

update according to @bulislaw 's review

261f74c

donatieng previously requested changes Nov 30, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Nov 30, 2018

fix according to @donatieng's review

6be8b54

ithinuel mentioned this pull request Jan 22, 2019

Pinmap extensions #9449

Merged

SenRamakri suggested changes Feb 8, 2019

View reviewed changes

c1728p9 mentioned this pull request Feb 12, 2019

Pinmap design doc #9690

Merged

cmonr closed this Feb 12, 2019

cmonr removed the needs: work label Feb 12, 2019


		## Description

		This update aims at increasing tests thoroughness by enabling them to check driver's reliability for each pin declared as supporting a peripheral.


		## Requirements

		1. We want to list all pins for a function on a peripheral.


		## Motivation

		At the time being, drivers are only tested on a single "default" peripheral. However, some target features the same peripheral through different IP for example the SPI may be provided on a single MCU by its USART, QSPI and SSP peripherals. To ensure that the driver implementation is valid for all these peripherals we want the CI to assess each pin in at least one pin configuration for each peripheral.

Add RFC for PinMap's extension #8693

Add RFC for PinMap's extension #8693

Uh oh!

Conversation

ithinuel commented Nov 9, 2018

Description

Pull request type

Uh oh!

adbridge commented Nov 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ithinuel Nov 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fkjagodzinski Nov 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fkjagodzinski left a comment

Choose a reason for hiding this comment

Uh oh!

bulislaw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donatieng commented Nov 22, 2018

Uh oh!

bulislaw commented Nov 22, 2018

Uh oh!

cmonr commented Nov 26, 2018

Uh oh!

ithinuel Nov 15, 2018 •

edited

Loading

fkjagodzinski Nov 15, 2018 •

edited

Loading

donatieng left a comment •

edited

Loading