-
Notifications
You must be signed in to change notification settings - Fork 3k
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
UBLOX_C027: init us_ticker in the target initialization #7060
Conversation
@ARMmbed/mbed-os-wan Can you please review and test if this fixes the issue was reported earlier? cc @ARMmbed/team-ublox |
Tested locally and I can verify that this fixes the hardfault issue. |
That should go into RC2. |
@@ -42,6 +42,7 @@ void ublox_mdm_init(void) | |||
// Can't use wait_ms() as RTOS isn't initialised yet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clean up the comments to describe what is actually happening? For example, would using wait_us have solved the problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I don't know what is happening and why there is a delay here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This board needs yet to be ported by Ublox to the new APIs (ticker mandates us_ticker_init needs to be called before any other us ticker calls).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hadn't spotted that this was a requirement of the new ticker API, had kinda assumed that any changes were backwards compatible. Obviously not. Are there any other things of this nature that we have missed? Note that, for reasons I've never understood, you don't run C027 on your test farms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Rob, right now C027 does not support us ticker, lp ticker and RTC. Porting guides are here:
https://os.mbed.com/docs/v5.8/reference/microsecond-ticker.html
https://os.mbed.com/docs/v5.8/reference/low-power-ticker.html
https://os.mbed.com/docs/v5.8/reference/rtc-port.html
Please note they need to be updated to reflect that the changes are live on master right now and will be part of 5.9, not longer on feature branches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, I might be confused; the us_ticker_read()
call has been there for a long time, hence should work with the old usTicker API but it seems that's not the case as this error has occurred. I can see what we should move to the new one, am wondering what happened to the old one...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old API is not longer there, we don't maintain two versions at the same time. The calls are for most part the same, just some additional requirements are in place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this closer, us_ticker_read
seems to be a device hal API where the platform API seems to be ticker_read_us
which starts the ticker if not already started. wait_us
still yields to the RTOS so not a good option for where this needing to be called from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I see at least one of my sources of confusion: C027 defines the macro TARGET_LPC1768
but doesn't actually inherit the target LPC1768
, just the target LPCTarget
, which doesn't do much of interest; hence the C027 target needs to add any "device_has"
macros all by it's little 'ole self (I'm used to the STM targets which are much more hierarchical). We will add support for USTICKER
.
Now to my other source of confusion: since there is a ticker_api
and a us_ticker_api
in HAL, both of which serve our purpose for this case (since we can read microseconds from them), which would be the correct one to use in this "mid-boot" state, or doesn't it matter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ticker_api is just a "parent" to us_ticker_api and lp_ticker_api. The whole timers/tickers abstraction is a bit complicated, but basically it's your standard object oriented abstract/virtual inheritance. You can use all of the high level classes with any ticker that inherits from ticker_api. So to instantiate us ticker in terms of ticker_api you use get_us_ticker_data
and then interact with that using ticker_api.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/morph build |
Build : SUCCESSBuild number : 2227 Triggering tests/morph test |
Test : SUCCESSBuild number : 2020 |
Exporter Build : SUCCESSBuild number : 1855 |
Description
A hardfault for this target has been reported.
Pull request type