ncs36510: us ticker improvements

[0xc0170]

Do not ticker read in ISR, use reminder to schedule the next interrupt, this should make ticker much faster.

read - disable Timer0 while reading it, if ISR is pending just reread the time again via read() function

These 2 improvements should decrease time spent when reading/scheduling ticker
events.

Tested with ticker test for GCC ARM and ARM, multiple times. Prior this patch , the times were at the limit thus ticker would fail seldom (quite often). With this patch, the ticker reading is much faster (not only reading the t ime, but also ISR as it was reading previously thus was slow) and we are for ticker in the middle of the range time.

@studavekar @c1728p9 @geky Please review, can cherry pick this to host time improvements, should fix the failure there and make it green I hope !

[0xc0170]

cc @radhika-raghavendran @pradeep-gr

[c1728p9]

This seems like it could be a more general problem with handling overflows. What are your thoughts on adding a simple test to the HAL to deterministically catch this? Something simple like reading non-stop in a loop might make catching this easier.

[c1728p9]

What are your thoughts on refactoring this into a loop? Some of the extra logic, like calling NVIC_DisableIRQ only needs to be done once. Also, having a loop also makes it clear that there can't be a stack overflow.

[0xc0170]

Ah should just reread the ticker, I rebased, fixed

[0xc0170]

This seems like it could be a more general problem with handling overflows. What are your thoughts on adding a simple test to the HAL to deterministically catch this? Something simple like reading non-stop in a loop might make catching this easier.

Would be good to have! Can you provide a code snippet for the reading non-stop in the loop?

[c1728p9]

I was thinking something like this:

uint32_t last_time = us_ticker_read();
for (uitn32_t i = TEST_LOOPS; i > 0; i--) {
    uint32_t cur_time = us_ticker_read();
    TEST_ASSERT(cur_time >= last_time);
    last_time = us_ticker_read();
}

That way if there is a glitch with handling overflows this test will trigger an assert since time will have gone into the past.

[geky]

Ah wait, shit, I had a patch that I forgot to pr. With @pan-'s 64-bit timer patch (#4094), you can throw out a whole bunch of the logic. We can just provide a 16-bit timer and leave the software timer to the higher-level: geky@492b558

I can put that patch up as a pr after this goes in, but I'm not sure how it affects your patch. Feel free to just merge it into this pr if it helps.

EDIT: Nevermind, @c1728p9 pointed out my implementation was subtly flawed. When the 16-bit timer overflows the higher-level thinks there was a 32-bit overflow and jumps 4295 seconds into the future. 😆

[0xc0170]

retest uvisor

[0xc0170]

/morph test

[mbed-bot]

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 719

All builds and test passed!

[0xc0170]

@mazimkhan Please restart uvisor? seems like not reporting status for various PR (24hours or more)

[0xc0170]

EDIT: Nevermind, @c1728p9 pointed out my implementation was subtly flawed. When the 16-bit timer overflows the higher-level thinks there was a 32-bit overflow and jumps 4295 seconds into the future. 😆

Yes, I tried your patch earlier, did not work properly, tried to improve it, failing sometimes. us ticker currently requires to have 32bit timer, so I left the implementation with 2 tickers. Thanks for the reminder.

Waiting for uvisor, this patch is fixing current ticker tests in our codebase, should get in asap to fix sporadic CI failures for this target.

[0xc0170]

@c1728p9 I added us ticker as separate PR, should not block this PR. here's a proposal for us ticker test #4690