Add the porting guide for the explicit pin-map extension. #1156
Conversation
mprse
force-pushed
the
explicit_pinmap_porting_guide
branch
from
d59a3b9 to
5b543f0
Compare
|
cc @jamesbeyond |
|
ping @jamesbeyond |
1 similar comment
|
ping @jamesbeyond |
|
Sorry for the later review.
|
|
Fixed as suggested. |
There was a problem hiding this comment.
I am happy about the contents of the docs. @bulislaw please review
docs/porting/target/static_pinmap.md
Outdated
| HAL APIs making use of pins take these pins in their constructor and use those pins to lookup which peripheral/function to use. The process of looking up the peripheral/function requires there to be a pinmap table that maps pins to peripherals/functions. This pinmap table takes up ROM which could be saved if the pinmap wasn't used. Static pinmap extension provides additional HAL API/constructors which takes pinmap as a parameter where pin/peripheral/function is specified staticly and there is no need to use the pinmap tables. | ||
|
|
||
| Supported peripherals: | ||
| - `PWM` |
There was a problem hiding this comment.
What about digitalio? I mean it would make a lot of sense to say Pins p23, p24 DigitalOut; p12 DigitalIn; Pins p34, 35 UART; all the other pins not used In this case we would only need descriptors for 5pins and ignore the remaining ones saving further memory.
There was a problem hiding this comment.
I'm not sure which, if any, platforms would benefit.
Most perform a fairly simple computation from pin number to GPIO bit/peripheral (because selecting GPIO is generally uniform across all pins), and the static information they write into their gpio_t is a bit more varied than doing the basic muxing of "set function 3" when selecting an alternate function - they're not using our PinMap type. The "pin/peripheral/function" 3-ple probably wouldn't cut it.
What might make sense is the option of a target constexpr gpio_t-returning function, and then have something like
struct gpio_pinmap_t {
#ifdef HAVE_STATIC_GPIO
gpio_t gpio;
#else
PinName pin;
#endif
};
constexpr gpio_pinmap_t gpio_get_pinmap(PinName pin)
{
#ifdef HAVE_STATIC_GPIO
return { hal_get_gpio(PinName pin) };
#else
return { pin };
#endif
}
DigitalIn::DigitalIn(const gpio_pinmap_t &map)
#ifdef HAVE_STATIC_GPIO
: gpio(map.gpio)
#endif
{
#ifndef HAVE_STATIC_GPIO
gpio_init(&gpio, map.pin);
#endif
...
}
constexpr gpio_pinmap_t pin_a_map = gpio_get_pinmap(PIN_A);
DigitalIn pin_a(pin_a_map);
No idea whether that really saves anything significant in practice. It might be worse on some platforms.
There was a problem hiding this comment.
As far as I remember only some Freescale targets have pin-maps for GPIOs, but even these targets do not use them during initialization. Probably they are redundant.
| Use code below to check if static pinmap extension works. | ||
|
|
||
| ``` | ||
| int main() |
There was a problem hiding this comment.
Do we need (or maybe already have) a test for that in HAL suite?
There was a problem hiding this comment.
I added the test cases which use static pinmap to the FPGA tests. Unfortunately, we do not have tested CAN, QSPI and AnalogOut (this should be added later, because for now Analogout test has been removed due to some issue with the FPGA test shield).
|
What's the relevant code PR for this? Has it already merged? |
docs/porting/target/static_pinmap.md
Outdated
| mbed test -t ARM -m K64F -n tests-mbed_hal_fpga_ci_test_shield* | ||
| ``` | ||
|
|
||
| <span class="notes">**Note:** Your target must be ready to run FPGA-Test-Shield tests. You can test the following peripherals: `Analogin`, `SPI`, `I2C`, `PWM`, `UART`.</span> |
There was a problem hiding this comment.
Is this list likely to change? If so and it's easy for it to become out of date, I suggest removing it.
There was a problem hiding this comment.
I will remove the list.
| | targets\TARGET_Freescale | 16581(-816) | 12(+0) | 340(+0) | // remove pin maps and driver code reduction | ||
| | Subtotals | 44290(-1010) | 264(+0) | 205518(+0) | | ||
| Total static RAM memory (data + bss): 205782(+0) bytes | ||
| Total flash memory (text + data): 44554(-1010) bytes |
There was a problem hiding this comment.
Are these numbers constant, or do they differ by board, configuration and so on? If the latter, I recommend removing it.
There was a problem hiding this comment.
For sure these are not constant, but the specific values are irrelevant here. The comments are important which shows in which modules we expect memory changes when static pinmap is used.
docs/porting/target/static_pinmap.md
Outdated
|
|
||
| ## Implementing static pin map extension | ||
|
|
||
| Most of the above points are already implemented. To make static pin map available on your platform, please perform the following steps: |
There was a problem hiding this comment.
If the above points are already implemented, do we still need to tell people to do them? Should we remove them?
There was a problem hiding this comment.
I will try to modify this part and leave only the information required for porting.
docs/porting/target/static_pinmap.md
Outdated
|
|
||
| Most of the above points are already implemented. To make static pin map available on your platform, please perform the following steps: | ||
|
|
||
| - Provide implementation of `xxx_init_direct(xxx_t *obj, static_pinmap_t *)` function (which does not use pin map tables). |
There was a problem hiding this comment.
Do these bulleted steps need to happen in order?
There was a problem hiding this comment.
Nope, but all together :-)
docs/porting/target/static_pinmap.md
Outdated
|
|
||
| The tables are required in the header file, so constant expression utility functions can include and use them to find and return mapping without pulling the pin map table into the image. | ||
|
|
||
| <span class="notes">**Note:** Please include the `<mstd_cstddef>` module, and use the `MSTD_CONSTEXPR_OBJ_11` macro instead of the `constexpr` specifier. This must be done for backward compatibility with the Arm 5 compiler, which does not support constant expressions. When you use the Arm 5 compiler, `MSTD_CONSTEXPR_OBJ_11` translates to `const`.</span> |
There was a problem hiding this comment.
Do you only need to do this when using the Arm 5 compiler?
There was a problem hiding this comment.
This is required generally, but it is used to deal with ARM 5 compiler. If we use constexpr, then we will get an error in the case of ARM 5 compiler. If we use MSTD_CONSTEXPR_OBJ_11 macro we will get in case of ARM 5 const (instead constexpr which will be used in all other cases).
There was a problem hiding this comment.
This wasn't actually anything to do with ARM C 5 - it was to handle the case where the header was going to be included from C code (inside the target code).
When we include it from the Mbed OS C++ code, we need to see it as constexpr, but if the target code includes it from C, it has to have it as const.
There was a problem hiding this comment.
Yes, but I remember that this was also related to ARMC5:
ARMmbed/mbed-os#11399 (comment)
But this was about the constexpr functions and here we have constexpr tables.
I mixed this. Thanks for catching this.
docs/porting/target/static_pinmap.md
Outdated
|
|
||
| ## Assumptions | ||
|
|
||
| 1. Provide types that will hold static pin maps for peripherals (`PWM`, `AnalogIn`, `AnalogOut`, `SPI`, `I2C`, `UART`, `QSPI`, `CAN`). |
There was a problem hiding this comment.
Do these numbered steps need to happen in order?
There was a problem hiding this comment.
No. This will be changed as you suggested.
| @@ -0,0 +1,180 @@ | |||
| <h1 id="static-pinmap-port">Static Pinmap extension</h1> | |||
|
|
|||
| The **Static Pinmap extension** allows you to statically specify the peripheral configuration (pin, peripheral or function) in the HAL API function. | |||
There was a problem hiding this comment.
Is this a HAL API, or is an extension something different? If it is an API, we should link to the Doxygen in the implementing section.
There was a problem hiding this comment.
The HAL API will be extended: xxx_init_direct() functions will be added (e.g. spi_init_direct()). Also, the driver's layer will be extended and we will have additional constructors that will use xxx_direct() HAL API.
Which link should be added?
https://os.mbed.com/docs/mbed-os/v5.14/apis/spi.html
I'm not sure how these docs are updated. At the moment there is no constructor that uses xxx_direct() HAL API on the list. Will it be added automatically when static pinmap is merged into the master?
There was a problem hiding this comment.
Which HAL API will be extended? SPI?
When new APIs are added with the right Doxygen tags and doxyfile options (I think here: https://github.com/ARMmbed/mbed-os/blob/master/doxygen_options.json), they show up in the Doxygen: https://os.mbed.com/docs/mbed-os/v5.14/mbed-os-api-doxy/annotated.html
There was a problem hiding this comment.
The following HAL and Drivers (classes) APIs will be extended:
- SPI
- I2C
- Serial
- PWM
- AnalogIn
- AnalogOut
- CAN
- QSPI
|
|
||
| In modern MCUs, you can often map peripherals to different pins, and each pin can have multiple functions. Mbed supports dynamic pin mapping, meaning you can reconfigure pins at run time for different drivers to use. That provides great flexibility, but it's not free. There's a nontrivial ROM cost to maintain the pin map tables and infrastructure to parse it. In some use cases, this flexibility is worth the cost. Often, pin configuration is frozen at the hardware design stage and doesn't require run time modification. Shifting this configuration to compile time allows free memory associated with the dynamic approach. | ||
|
|
||
| HAL APIs using pins take these pins in their constructor and use those pins to look up which peripheral or function to use. The process of looking up the peripheral or function requires there to be a pin map table that maps pins to peripherals or functions. This pin map table takes up ROM. Static pinmap extension provides additional HAL API constructors, which take pin map as a parameter where the pin, peripheral or function is specified statically, and there is no need to use the pin map tables. |
There was a problem hiding this comment.
So does this mean the benefit of the static pin map extension is that it saves ROM?
There was a problem hiding this comment.
Correct. This is the main reason for adding this.
https://github.com/ARMmbed/mbed-os/tree/feature-hal-spec-explicit-pinmap This is on the feature branch. One PR is left and when it is merged we can bring this to master. |
mprse
force-pushed
the
explicit_pinmap_porting_guide
branch
from
fc679b7 to
ca62a4e
Compare
|
@AnotherButler I addressed all the review comments except adding a link to the Doxygen. |
|
I think my changes were overwritten. Could you please reinstate them? |
Make initial edits, mostly for active voice, formatting and spelling.
mprse
force-pushed
the
explicit_pinmap_porting_guide
branch
from
e3ab513 to
108effe
Compare
Ohhh, sorry about this. I couldn't cherry-pick the lost commit so I tried to add your edits manually. Hopefully, it is ok now. |
|
Thanks. That looks good 👍 When do we expect the code to merge to master? 5.15 or later? |
|
PR ARMmbed/mbed-os#11892. |
|
In that case, could an engineer please add it to the Doxyfile options in the |
PR ARMmbed/mbed-os#11892 has been merged and it's tagged for So as far as I understand we expect that new direct init functions will show up here after release: Then I will be able to add a link to them to the porting guide. @AnotherButler It that correct? |
Add the porting guide for the explicit pin-map extension.