STM32: Add LP_TICKER and US_TICKER optimisation macro #14536
Conversation
ciarmcom
added
the
release-type: patch
|
@jeromecoutant, thank you for your changes. |
| @@ -22,6 +22,9 @@ | |||
| #include "PinNames.h" | |||
| #include "gpio_object.h" | |||
|
|
|||
| #include "us_ticker_defines.h" | |||
| #include "lp_ticker_defines.h" | |||
There was a problem hiding this comment.
Eh? Why wasn't us_ticker_defines already there?
Oh, looks like this was broken in 4740775 from #13090.
So the wait_us optimisations have been broken for STM since Mbed OS 6.1, and the custom-tickers stuff would never have been doing anything.
This should be included in the documentation - you are also re-adding the missing USTICKER optimisations.
But only for STM32F0 in this version.
You should put the usticker include up in device.h, as it's a shared not-target-specific header, to reactivate it for all devices. (That would be reverting #13090 in its entirety.)
I guess you could put the usticker include in every objects.h instead, which would have made the custom target with replacement device.h work in #12990 but I'm not sure what was being done there (replacing device.h) was in any sense documented.
There was a problem hiding this comment.
@kjbracey-arm
Updates done for all STM32
| #define LP_TICKER_PERIOD_NUM 1000000 | ||
|
|
||
| #if MBED_CONF_TARGET_LSE_AVAILABLE | ||
| #define LP_TICKER_PERIOD_DEN (LSE_VALUE / 4) // RTC_CLOCK / RTC_WAKEUPCLOCK_RTCCLK_DIV4 |
There was a problem hiding this comment.
What are the comments trying to say here?
If that's a better way of writing the expression, why not use it?
If you can't due to non-visibility of those macros in general context, so you're writing something you think should be equivalent, how about asserting equivalence in a C file where you can?
#if LP_TICKER_PERIOD_DEN != RTC_CLOCK / RTC_WAKEUPCLOCK_RTCCLK_DIV4
#error "Mismatch with lp_ticker_defines.h"
#endif
Although I guess the lp_ticker_info_test would catch it with its runtime cross-check.
| ****************************************************************************** | ||
| */ | ||
|
|
||
| #ifndef __LP_TICKER_DEFINES_H |
There was a problem hiding this comment.
Given that lp_ticker.c is common for all STM families, can't this file be common for all families too, like us_ticker_defines.h?
Would just need the extra ifdefs for DEVICE_LPTICKER and MBED_CONF_TARGET_LPTICKER_LPTIM you see in the C file.
jeromecoutant
force-pushed
the
DEV_STM32F0_LPTICKER_OPTIM
branch
from
f92125c
to
f95babb
Compare
jeromecoutant
changed the title
| @@ -37,4 +37,7 @@ | |||
|
|
|||
| #include "objects.h" | |||
|
|
|||
| #include "us_ticker_defines.h" | |||
| #include "lp_ticker_defines.h" | |||
There was a problem hiding this comment.
Could you make this include, or the contents of the header file, conditional on DEVICE_LPTICKER or whatever it is?
I'd rather the macros were left undefined in that case. There may not be any problem in the Mbed OS tree, but it's possible someone might assume LPTICKER is available if LP_TICKER_PERIOD_NUM is defined.
There was a problem hiding this comment.
@kjbracey-arm
Done
jeromecoutant
force-pushed
the
DEV_STM32F0_LPTICKER_OPTIM
branch
from
f95babb
to
45a6787
Compare
|
CI started |
Jenkins CI Test : ❌ FAILEDBuild Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
I forgot to indicate this PR needs #14554 first |
|
Let's restart CI ? |
|
CI restarted |
Jenkins CI Test : ❌ FAILEDBuild Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
Seems to be an issue with Ethernet tests, which is not related to this PR :-( |
Jenkins CI Test : ❌ FAILEDBuild Number: 3 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
|
Let's restart CI ? |
|
This pull request has automatically been marked as stale because it has had no recent activity. @jeromecoutant, please carry out any necessary work to get the changes merged. Thank you for your contributions. |
|
Let's restart CI ? |
|
CI started |
Jenkins CI Test : ✔️ SUCCESSBuild Number: 4 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
mbedmain
added
release-version: 6.12.0
Release-pending
and removed
release-type: patch
Summary of changes
Proposition is following:
This needs an update in hal/lp_ticker_info_test, as tested expression could exceed u32.And made for the patch in hal/us_ticker_info_test@kjbracey-arm
Impact of changes
Migration actions required
Documentation
Pull request type
Test results
Reviewers