Add explicit pin-map support for K64F and NUCLEO_F429ZI #11399
Conversation
ciarmcom
requested review from
0xc0170,
fkjagodzinski,
jamesbeyond,
kjbracey,
maciejbocianski and
a team
|
@mprse, thank you for your changes. |
|
Nice work. Initial comments before detailed review... Before finalising this, I would like confirmation that the proposed Make at least one test (You can knock it together standalone outside the HAL for now in a test app, duplicating whatever tables are necessary). For the lack of saving on that target - can you attach the respective before/after map files here, or stick them up somewhere? |
I will do that. Unfortunately, I noticed that I missed adding explicit pinmap support for serial flow control. I will add this missing part and then I'll try to use
This archive contains map files, elf files, and output from |
|
The image is 968 bytes smaller in those map files - the base one has text size If that isn't showing up as a 968-byte difference, something in the STM layout must be confusing your memory analysis tool. I think. |
|
@trowbridgec @AGlass0fMilk check this out |
mprse
force-pushed
the
explicit_pinmap_gold
branch
from
6d48c21 to
d94c076
Compare
|
Added missing support for serial flow control. |
|
Please check Travis failures |
|
Added Below memory usage analysis. The results are slightly different than the previous since:
Example memory usage change for |
|
@kjbracey-arm Can you please review. |
hal/explicit_pinmap.h
Outdated
|
|
||
| constexpr PinMap get_analogin_pinmap_t(const PinName pin) | ||
| { | ||
| for (uint32_t i = 0; i < (sizeof(PINMAP_ANALOGIN)/sizeof(PINMAP_ANALOGIN[0])); i++) { |
There was a problem hiding this comment.
Now we're in C++14 land, this can be
for (const PinMap &pinmap: PINMAP_ANALOGIN) {
if (pinmap.pin == pin) {
return pinmap;
}
}
Or you could go nuts and do this:
PinMap *pinmap= std::find_if(std::begin(PINMAP_ANALOGIN), std::end(PINMAP_ANALOGIN),
[&](const PinMap &e) { return e.pin == pin });
if (pinmap) {
return *pinmap;
}
But that latter more-verbose form only makes sense if you were really thinking PINMAP_ANALOGIN might be some sort of container which wanted to do a faster-than-linear search for performance reasons, so forget I even mentioned it...
There was a problem hiding this comment.
Fixed, but only for peripherals with one pin. It seems that when more pin tables need to be seach and the results compared the current version is better.
hal/explicit_pinmap.cpp
Outdated
| @@ -23,6 +23,8 @@ | |||
| #include "analogout_api.h" | |||
| #include "i2c_api.h" | |||
| #include "serial_api.h" | |||
| #define CONSTEXPR constexpr | |||
There was a problem hiding this comment.
As this is local trickery, I'd be inclined to call it PINMAP_CONSTEXPR or something, to make sure not to collide with someone else doing something similar.
Alternatively, maybe you could incorporate something like this into <mstd_cstddef>? There are some "C++11 or C++14" macros there - maybe we could have "C++11 or C99/C++98" macros to go with them? Make that header work in C mode, and define MSTD_CONSTEXPR_OBJ as const or constexpr accordingly.
There was a problem hiding this comment.
Although, I don't think you technically need to actually mark those tables as constexpr anyway. As long as they are constant expressions, these function definitions should work and be constexpr.
Putting the constexpr in the table just forces a pre-check - you'd get a compile error from the tables themselves if you put something non-constant in.
I guess it's kind of nice to force the check - with just const there is a chance the table silently goes into RAM if not a constant expression, but constexpr blocks it. But we've managed without that check up until now, so I wouldn't sweat it too much.
There was a problem hiding this comment.
I checked that and it looks like we need constexpr for the pinmap tables:
Compile [ 99.8%]: explicit_pinmap.cpp
[Error] explicit_pinmap.h@22,18: constexpr function never produces a constant expression [-Winvalid-constexpr]
[ERROR] In file included from .\hal\explicit_pinmap.cpp:27:
./hal/explicit_pinmap.h:22:18: error: constexpr function never produces a constant expression [-Winvalid-constexpr]
constexpr PinMap get_pwm_pinmap_t(const PinName pin)
^
./hal/explicit_pinmap.h:25:13: note: read of non-constexpr variable 'PinMap_PWM' is not allowed in a constant expression
if (PINMAP_PWM[i].pin == pin) {
^
./targets/TARGET_Freescale/TARGET_MCUXpresso_MCUS/TARGET_MCU_K64F/TARGET_FRDM\PeripheralPinMaps.h:373:20: note: expanded from macro 'PINMAP_PWM'
#define PINMAP_PWM PinMap_PWM
There was a problem hiding this comment.
You're right. GCC does accept it, but clang refuses, and it's allowed to.
Only const ints or enumerations can be used in constant expressions - you can't actually access an array of ints. That's one of those very early C++ things where they treated const int specially to try to get people to stop using #defines.
There was a problem hiding this comment.
I was misled by the answer here: https://stackoverflow.com/questions/14116003/difference-between-constexpr-and-const
because I missed the clause "of integral or enumeration type" in the bit about "an object can be used as constant expression without being declared constexpr".
There was a problem hiding this comment.
Fixed. Used MSTD_CONSTEXPR_OBJ_C14 macro.
hal/explicit_pinmap.h
Outdated
| return {PINMAP_UART_TX[tx_idx].peripheral, PINMAP_UART_TX[tx_idx].pin, PINMAP_UART_TX[tx_idx].function, PINMAP_UART_RX[rx_idx].pin, PINMAP_UART_RX[rx_idx].function}; | ||
| } | ||
|
|
||
| constexpr serial_fc_pinmap_t get_uart_fc_pinmap_t(const PinName rxflow, const PinName txflow) |
There was a problem hiding this comment.
Still think these functions shouldn't have the _t suffix. You're getting the pinmap, which has type pinmap_t. Only types should have the _t suffix.
hal/explicit_pinmap.h
Outdated
| #define CONSTEXPR constexpr | ||
| #include "PeripheralPinMaps.h" | ||
|
|
||
| constexpr PinMap get_pwm_pinmap_t(const PinName pin) |
There was a problem hiding this comment.
These functions will need to use the MSTD_CONSTEXPR_FN_14 macro from <mstd_cstddef> to not fall over with ARMC5. ARMC5 doesn't support C++14 constexpr, so these functions would not in fact be constexpr for it, and would fail to compile.
Obviously then these functions would not save ROM on ARMC5, as they'd pull in the runtime tables, but that just needs to be documented. Can't break compilation altogether.
mprse
force-pushed
the
explicit_pinmap_gold
branch
from
7f17564 to
48ad43a
Compare
|
|
||
| {NC , NC , 0} | ||
| }; | ||
| } |
| #endif | ||
|
|
||
| /************GPIO***************/ | ||
| MSTD_CONSTEXPR_OBJ_14 PinMap PinMap_GPIO[] = { |
There was a problem hiding this comment.
This is sort of the right idea, but it's misusing the macro a bit. The 14 macros are intended to be "this is constexpr if C++14 or later". This stuff is "constexpr if C++11 or later". (Even if the mapping function isn't constexpr until C++14, these tables can be constexpr in C++11).
You could define that yourself as
#if __cplusplus >= 201103 // I think that's the right number
#define MSTD_CONSTEXPR_OBJ_11 constexpr
#else
#define MSTD_CONSTEXPR_OBJ_11 const
#endif
but I would rather put that in mstd_cstddef itself (and adjust it to work from C). I think you won't be the only person wanting to do this in a C-or-C++ header, so let's add it to the infrastructure. (And the FN version, although that would need a little more care if people used it from C).
I didn't put this in originally because I was trying to minimise magic macros, and I was assuming all C++ would be C++11 or later, so just constexpr would do, but I didn't consider shared C-and-C++ headers.
|
|
||
| const PinMap explicit_pinmap = {pin, peripheral, 0}; | ||
|
|
||
| analogin_init_direct(obj, &explicit_pinmap); |
There was a problem hiding this comment.
You could use a C99 compound literal here:
analogin_init_direct(obj, &(const PinMap) { pin, peripheral, 0 });
Question of taste.
|
|
||
| // STDIO for console print | ||
| #ifdef MBED_CONF_TARGET_STDIO_UART_TX | ||
| # define STDIO_UART_TX MBED_CONF_TARGET_STDIO_UART_TX |
There was a problem hiding this comment.
Not objecting to the change, but wondering why if there's a particular reason these are becoming macros? Just to line up with the FUNC macros?
There was a problem hiding this comment.
Because I'm using macros to select how the console should be initialized. If all data required for explicit pin-map is defined, then we use explicit pin-map to init console:
mbed-os/platform/source/mbed_retarget.cpp
Line 282 in 48ad43a
But now we could use constexpr utility functions to do the job and remove these FUNC macros.
There was a problem hiding this comment.
No #define STDIO_UART_TX_FUNC in case of MBED_CONF_TARGET_STDIO_UART_TX ?
There was a problem hiding this comment.
MBED_CONF_TARGET_STDIO_UART_XX comes from the config file, so in such case, we would have to extend the config file. I think that these extra macros can be removed now and we can use constexpr utility functions to find mappings. I'm checking this now.
There was a problem hiding this comment.
If all data required for explicit pin-map is defined, then we use explicit pin-map to init console:
Gotcha. The inability to test for enums (plus people using enums) drives me nuts in HAL-type code.
I don't particularly fancy the maintenance burden of maintaining the pinmap tables AND those separate defines. I think I'd kind of like to just pressure people to do the full constexpr thing - "if you have the constexpr stuff, retarget will use it, so your image will get smaller".
I did a parallel define thing for the us_ticker optimisations, and that's also ugly, but there is no "plan B" alternative yet... (Actually "plan A" - the defines are "plan B").
If those defines do stay, I'd want to, if possible, have a cross-check that they're correct. Have a Greentea test that checks the run-time lookup for the pins matches any declared defines. That was done for the us_ticker optimisation - defines vs us_ticker_info structure.
There was a problem hiding this comment.
I covered this in passing in a comment I made an hour ago. The second form prompts the compiler to generate code to put the (precomputed) spi_pinmap_t on the stack and passes a stack pointer to the function. The first form just has the precomputed pinmap in ROM and points to it.
So the first form can generate smaller code. Not sure how IAR makes it bigger though.
It may be a technical requirement of the C++ standard that it goes onto the stack - distinct objects may have to have distinct addresses, so it can't just make all calls with that pinmap point to the same thing.
(String literals can share addresses, as can C99 constant compound literals IIRC, but C++ constant temporaries can't. I think).
There was a problem hiding this comment.
Oh, but you'd want to make sure that separate spi_pinmap_t was static too, or it might put it on the stack anyway, if that's inside a function.
(Although in turn, if get_spi_pinmap wasn't constexpr, then static would cost you static RAM, as it would assemble it into static RAM on first call, rather than assembling it onto the stack on each call. Probably a RAM/ROM tradeoff there. But if get_spi_pinmap is constexpr, then static guarantees accessing it in ROM, rather than a stack copy, which almost certainly saves ROM and doesn't affect static RAM).
There was a problem hiding this comment.
static const serial_pinmap_t console_pinmap = get_uart_pinmap(STDIO_UART_TX, STDIO_UART_RX);
static DirectSerial console(console_pinmap, MBED_CONF_PLATFORM_STDIO_BAUD_RATE);
Adding static gives:
ARM: -22 BGCC_ARM: +0 BIAR: -16 B
There was a problem hiding this comment.
I found a problem while working on the serial console initialization using constexpr pin-map function. The solution works fine, but I lost backward compatibility.
At the moment console is initialized using explicit pin-map mechanism (without constexpr) if all required defines are available: STDIO_UART_TX, STDIO_UART_TX_FUNC, STDIO_UART_RX, STDIO_UART_RX_FUNC, STDIO_UART. Otherwise, we use the old version which uses pin-map tables.
Now when I'm using contsexpr function to find mapping I need to have peripheralPinMaps.h files ready for all targets.
Maybe a good solution would be to add some configuration macro for targets which support explicit pin-maps in targets.json. Something like: device_has: "EXPLICIT_PINMAP" ?
This could be removed when explicit pin-map support is added to all targets, but for now, I could add support in parts.
There was a problem hiding this comment.
I'd put a indicator macro in PinNames.h that you have the extended HAL functionality. That's similar to what I did in #10609 (that was new macros in device.h). Don't really need to drag the JSON into it, as it's an extension to something you can already find the main header for.
hal/explicit_pinmap.h
Outdated
| int tx_idx = NC; | ||
| int rx_idx = NC; | ||
|
|
||
| for (uint32_t i = 0; i < (sizeof(PINMAP_UART_TX)/sizeof(PINMAP_UART_TX[0])); i++) { |
There was a problem hiding this comment.
If you wanted to extend these to the ranged for, it would be
const PinMap *tx_map = nullptr;
for (const PinMap &pinmap : PINMAP_UART_TX) {
if (pinmap.pin == tx) {
tx_map = &pinmap;
break;
}
}
But at that point, the std::find_if version is actually more concise:
const PinMap *tx_map = std::find_if(std::begin(PINMAP_UART_TX), std::end(PINMAP_UART_TX),
[tx](const PinMap &map) { return map.pin == tx; });
const PinMap *rx_map = std::find_if(std::begin(PINMAP_UART_RX), std::end(PINMAP_UART_RX),
[rx](const PinMap &map) { return map.pin == rx; });
if (!tx_map || !rx_map || tx_map->peripheral != rx_map->peripheral) {
return {NC, NC, NC, NC, NC};
}
return {pinmap_tx->peripheral, pinmap_tx->pin, pinmap_tx->function, pinmap_rx->pin, pinmap_tx->function};
Still a bit clunky, so you could factor it out to a helper:
template<size_t N>
constexpr const PinMap *pinmap_lookup_pin(const PinMap(&maptable)[N], PinName pin)
{
// or use the range-loop here if you prefer. Important point was passing
// the array by reference to capture its size for the iteration.
return std::find_if(std::begin(maptable), std::end(maptable),
[pin](const PinMap &map) { return map.pin == pin; });
}
const PinMap *tx_map = pinmap_lookup_pin(PINMAP_UART_TX, tx);
const PinMap *rx_map = pinmap_lookup_pin(PINMAP_UART_RX, rx);
There was a problem hiding this comment.
Actually, I didn't realise that std::find_if isn't constexpr in C++14 - you'll have to use the ranged-for loop. The helper function would still make it neater though.
template<size_t N>
constexpr const PinMap *pinmap_lookup_pin(const PinMap(&maptable)[N], PinName pin)
{
for (const PinMap &pinmap : maptable) {
if (pinmap.pin == tx) {
return &pinmap;
}
}
return nullptr;
}
|
Just been experimenting a little in Compiler Explorer to see how much the separate pinmap object is needed with the constexpr - comparing with Turns out the compilers can eliminate the runtime pinmap tables in the second form, but will only do so under certain optimisation settings. GCC seems to do it at O1, O2 or Os, but not O0. Clang seems to do it at O2, but not at O0, O1, Oz or Os. It appears its "size" optimisation is eliminating enough inlining that the tables get pulled in, so Os is bigger than O2. (I tried an always_inline attribute, but didn't affect it). Also, the second form in both gcc and clang seems to insist on putting the temporary spi_pinmap_t on the stack, which means more code than just referencing a pre-built one in ROM. So the separate pinmap object form seems preferable to guarantee optimisation and make it smaller, but we could end up optimising existing users anyway, in some tools/profiles. (If you were to reorganise the constructors along those lines, so the pin ones are in the headers for inlining and delegate to the map ones). |
|
Further thought - not sure if it makes sense in terms of sequencing of changes/implementation, but This code - with I guess that does waste a bit of RAM if |
|
I think I addressed all the review comments. Last changes:
Test results: @kjbracey-arm Its ready for the next review round. Thank you for the valuable tips and help with the implementation. If this solution is accepted I will run |
mprse
force-pushed
the
explicit_pinmap_gold
branch
from
48ad43a to
22970fc
Compare
| #include <mstd_cstddef> | ||
|
|
||
| /************GPIO***************/ | ||
| MSTD_CONSTEXPR_OBJ_11 PinMap PinMap_GPIO[] = { |
There was a problem hiding this comment.
I think we're potentially hitting a problem with multiple tables ending up in ROM in pathological cases, but I'm not sure we can do anything about that in C++14.
If you had two source files each doing
get_spi_pinmap(pin_a, pin_b);
Where pin_a and pin_b are variables, then each of those files would have an inline get_spi_pinmap which accessed a local PinMap_SPI. (const objects have internal linkage, so each person including this header gets their own local copy).
Adding extern here wouldn't actually work, as it would then end up with multiple definitions.
What we actually need is inline, but that's C++17 for objects. Maybe time for a MSTD_INLINE_OBJ_17...
There was a problem hiding this comment.
This may happen only for ARMC5?
There was a problem hiding this comment.
I think this would be the issue for all compilers. We're safe as long as get_spi_pinmap is always called with constant parameters.
Fixing it properly requires compilers in C++17 mode so we can add inline to the objects.
The upshot is that it would not be safe to define SPI(PinName, PinName, ...) as an inline thing that calls get_spi_pinmap, and we should document that get_xxx_pinmap should only be used as a memory optimisation with constant parameters, otherwise the xxx(PinName...) forms should be used.
mprse
force-pushed
the
explicit_pinmap_gold
branch
from
22970fc to
3b665a5
Compare
targets/TARGET_STM/TARGET_STM32F4/TARGET_STM32F429xI/TARGET_NUCLEO_F429ZI/PinNames.h
Outdated
Show resolved
Hide resolved
…t (explicit pinmap).
adbridge
added
needs: CI
release-version: 6.0.0-alpha-1
mprse
force-pushed
the
explicit_pinmap_gold
branch
from
5986a6b to
958147e
Compare
|
Rebased this PR on top of If not we need to wait for the update of ARM compiler to |
|
Ci started on this to check if the rebase has fixed the issues |
Test run: SUCCESSSummary: 12 of 12 test jobs passed |
|
@jamesbeyond @adbridge @0xc0170 Can we merge this into the feature branch? This one is big enough. I'll continue working on this feature here: PR #11632. So potential change requests can be addressed there. |
adbridge
added
ready for merge
and removed
needs: CI
release-version: 6.0.0-alpha-1
Description
This PR adds explicit pin-map support for the
K64FandNUCLEO_F429ZIfor the all common peripherals like:Tables below show the ROM savings when the traditional pin-map mechanism is used to initialize the peripheral vs explicit pin-map mechanism is used:
In almost, all cases flash memory savings are consistent with the size of the pin-maps used by the peripheral. Additional savings can be also achieved by reducing the size of pin-map lib
(
mbed_pinmap_common.o). To get the max savings we need to completely remove the pin-maps, which means explicit pin-map must be used for serial console. In the above tables,serialrow includes also extra savings from removing pin-map lib completely(*), by initializing the console using explicit pin-map mechanism.Unfortunately in one case
GCC_ARM/NUCLEO_F429ZIthe results are very strange because there is no reduced memory (marked as red). I checked that pin-map tables were removed from elf, but this has no influence on the used memory. Does anyone maybe know why? Looks like it works forGCC_AMR/K64F.Below you can find detailed information how memory usage changes when example PWM application is build using traditional pin-maps (
master) and when the application uses explicit pin-maps (explicit_pinmap_gold):Sizes of the following modules have changed as follows (except
GCC_ARM/NUCLEO_F429Zno savings from removed pin-map tables):targets\TARGET_xxxxx(removed pin-map tables, driver modifications),hal\mbed_pinmap_common.o(reduced usage of functions from pin-map lib),main.o(application requires extra space for explicit pin-map structure).Example usage:
If the user tries to use the explicit pin-map mechanism on unsupported target, then will get the failure message:
Error Message: Explicit pinmap for SPI is unsupported on this device.Pull request type
Reviewers
@jamesbeyond @kjbracey-arm @0xc0170
@maciejbocianski @fkjagodzinski