npl/riot: Improve timer glue code #753

bergzand · 2020-02-10T12:43:56Z

This PR modifies and cleans the RIOT NPL glue code.

The first commit replaces the constants in the milllisecond-to-microsecond and microsecond-to-millisecond conversions to the RIOT defines for this to improve readability.
The second commit mitigates an incorrect remaining_ticks result when the RIOT microsecond timer overflows or when the Nimble thread is interrupted between setting the target_ticks and setting the timer.

@haukepetersen This might be of interest to you.

sjanc · 2020-02-11T12:12:38Z

porting/npl/riot/include/nimble/nimble_npl_os.h

@@ -215,7 +215,7 @@ ble_npl_callout_is_active(struct ble_npl_callout *c)
 static inline ble_npl_time_t
 ble_npl_callout_get_ticks(struct ble_npl_callout *co)
 {
-    return co->target_ticks;
+    return (ble_npl_time_t)co->target_ticks;


this looks a bit odd, shouldn't ble_npl_time_t in riot port be change to uint64_t ?

I agree that it is odd and there is a bug here.

I don't think it is necessary to change ble_npl_time_t to uint64_t. The main reason to change from uint32_t to uint64_t is in the places where NimBLE provides a time in milliseconds and RIOT expects it in microseconds. The conversion to microseconds should be paired with a conversion to uint64_t to compensate for the additional range provided by value as millisecond compared to microseconds.

The bug is this line is that target_ticks is not in milliseconds but in microseconds. The RIOT port specifies that 1 tick == 1 millisecond, so this function should also properly return a value in milliseconds and not in microseconds.

I prefer to stick to uint32_t for the ble_npl_time_t and make sure that values converted from microseconds to milliseconds wrap around at 2**32 milliseconds and not at 2**32/1000 milliseconds

fair enough, I'll leave final call to Hauke though :)

haukepetersen · 2020-02-13T09:43:07Z

@bergzand yes, this is indeed interesting :-)

Changes are valid and the code looks good to me, testing it as we speak.

haukepetersen

One more overflow situation we should consider.

Apart from that the changes look good, and all test still run as expected.

haukepetersen · 2020-02-13T10:00:57Z

porting/npl/riot/include/nimble/nimble_npl_os.h

@@ -100,7 +100,8 @@ ble_npl_eventq_get(struct ble_npl_eventq *evq, ble_npl_time_t tmo)
    } else if (tmo == BLE_NPL_TIME_FOREVER) {
        return (struct ble_npl_event *)event_wait(&evq->q);
    } else {
-        return (struct ble_npl_event *)event_wait_timeout(&evq->q, (tmo * 1000));
+        return (struct ble_npl_event *)event_wait_timeout(&evq->q,
+                                                          (tmo * US_PER_MS));


If I am not mistaken, a source for error remains for this call: if tmos value is greater than (UINT32T_MAX / 1000 -> 1,19 hours), the timeout will be set incorrectly. Looking at RIOTs µs-based timer APIs it seems this can not be so easily solved. But how about we a) put in an assert that triggers once this overflow happens, or b) return NULL in that case, which subsequently would also in most cases trigger an assertion by the calling code...

I have to admit that I intentionally ignored the edge case here, as you mention, there is no easy way to solve the edge case here.

I'm not that familiar with Bluetooth timings, do you consider it likely that this assertion will trigger during the lifetime of a node? Are there any timing durations in the Bluetooth spec itself that are longer than this 1.19 hours?

haukepetersen · 2020-02-13T10:20:03Z

One more: the overflow issues is also present in ble_npl_time_delay().

Thinking about his a little more, I think that we should actually expose fitting functions in RIOT so that we can cleanly map the effected functions:

expose xtimer_usleep64() in RIOT and use it to map ble_npl_time_delay() properly
add xtimer_set_timeout_flag64() and event_wait_timeout64 to the corresponding RIOT APIs and use them in ble_npl_eventq_get().

@bergzand what do you think?

bergzand · 2020-02-13T10:52:13Z

One more: the overflow issues is also present in ble_npl_time_delay().

These should show up only when the time in milliseconds causes an overflow in the uint32_t microsecond representation. (right?) Whereas with the ble_npl_callout_remaining_ticks it will always cause issues when the microsecond representation is near enough to UINT32_MAX.

I think adding assertions to ble_npl_time_delay() and ble_npl_eventq_get should be sufficient for now, assuming Nimble doesn't require timeouts longer than 1.19 hours in regular operations.

To mitigate this with a more long-term solution I think we either have to add the functions you've proposed to RIOT or merge and migrate to ztimer. ztimer allows for setting millisecond-based timers and would mitigate most of the issues present here.

haukepetersen · 2020-02-13T11:22:47Z

assuming Nimble doesn't require timeouts longer than 1.19 hours in regular operations.

for normal operation I don't think so, but I do not know for sure. I guess the assertions will telll...

I agree, that ztimer shoud be the mid-term goal. However, at least event_wait_timeout() would still prove to be a small challenge.

So how about:

catch overflows using asserts in ble_npl_time_delay() and ble_npl_eventq_get() for now, so we can get this PR merged and benefit from the fixes it provides
add API functions in RIOT to xtimer and event as intermediate
remove asserts from (1.) in favor of new functions
move to ztimer in general, once ztimer has stabilized a little further

bergzand · 2020-02-13T12:58:24Z

catch overflows using asserts in ble_npl_time_delay() and ble_npl_eventq_get() for now, so we can get this PR merged and benefit from the fixes it provides

Done

bergzand · 2020-02-13T14:06:13Z

And I think I broke something related to the announcements, I'll look into it

haukepetersen · 2020-02-13T14:23:39Z

Nice, I am just looking into doing the needed adaptions in RIOT for step 2...

bergzand · 2020-02-13T15:16:28Z

Nice, I am just looking into doing the needed adaptions in RIOT for step 2...

Thanks!

I think my code is fine, but somehow the RTT is broken on my test board. On a nRF52dk everything seems fine I receive the BLE advertisements.

haukepetersen

the adaption of RIOT has been quicker than anticipated :-)

haukepetersen · 2020-02-13T15:16:19Z

porting/npl/riot/include/nimble/nimble_npl_os.h

+        uint64_t tmo_us64 = tmo * US_PER_MS;
+        assert(tmo_us64 <= UINT32_MAX);
+        return (struct ble_npl_event *)event_wait_timeout(&evq->q,
+                                                          (tmo_us64));


in the current state, tmo_us64 should be explicitly casted to uint32_t I'd say

or we simply wait for RIOT-OS/RIOT#13373 to be merged :-)

Waited for RIOT-OS/RIOT#13373 and modified to event_wait_timeout64

haukepetersen · 2020-02-13T15:17:49Z

porting/npl/riot/include/nimble/nimble_npl_os.h

-    xtimer_usleep(ticks * 1000);
+    uint64_t us64 = ticks * US_PER_MS;
+    assert(us64 <= UINT32_MAX);
+    xtimer_usleep(us64);


getting RIOT ready was easier than expected: RIOT-OS/RIOT#13370

So we can drop the assert again and use xtimer_usleep64() directly here.

So we can drop the assert again and use xtimer_usleep64() directly here.

Done!

haukepetersen

Nice one, code wise just minor details left.

Will give this another set of decent test-runs tomorrow to make sure everything still behaves as expected...

haukepetersen · 2020-02-13T17:29:21Z

porting/npl/riot/include/nimble/nimble_npl_os.h

@@ -227,7 +228,7 @@ ble_npl_callout_set_arg(struct ble_npl_callout *co, void *arg)
 static inline uint32_t
 ble_npl_time_get(void)
 {
-    return xtimer_now_usec() / 1000;
+    return xtimer_now_usec64() / US_PER_MS;


just notice something that could be fixed while we are at it: ble_npl_time_get() should actually return a value of type ble_npl_time_t (see nimble/include/nimble/nimble_npl.h line 139), so we could just adapt the function declaration in this PR.

Furthre: it would be cleaner looking if the return value would be explicitly casted to ble_npl_time_t

return (ble_npl_time_t)(xtimer_now_usec64() / US_PER_MS);

haukepetersen · 2020-02-13T17:31:11Z

porting/npl/riot/src/npl_os_riot.c

-    time = xtimer_now_usec64() + ble_npl_time_ticks_to_ms32(timeout) * 1000;
-    abs.tv_sec = (time_t)(time / 1000000);
-    abs.tv_nsec = (long)((time % 1000000) * 1000);
+    time = xtimer_now_usec64() + ble_npl_time_ticks_to_ms32(timeout) * US_PER_MS;


nitpick: parenthesis would help readability here :-)

bergzand · 2020-02-13T19:48:26Z

I have 09b619b running on a device here connected to my phone with periodic BLE traffic between the two. I will leave it running overnight and see if anything pops up.

haukepetersen

Code looks good now, and all my tests still run as they are supposed to.

Feel free to squash, and lets hear what your experiment results for tonight were :-)

bergzand · 2020-02-14T10:04:48Z

Feel free to squash

Squashed!

and lets hear what your experiment results for tonight were :-)

Still connected to the phone as of now and I haven't detected any issues with the BLE connection.

haukepetersen · 2020-02-14T10:16:05Z

Perfect, then lets go as soon as Travis is happy as well.

bergzand · 2020-02-14T12:14:04Z

@haukepetersen All green!

haukepetersen · 2020-02-14T15:24:57Z

all green -> go.

bergzand · 2020-02-14T15:27:22Z

Thanks!

haukepetersen · 2020-02-14T15:57:14Z

Thank you, awesome work!

sjanc reviewed Feb 11, 2020

View reviewed changes

sjanc requested a review from haukepetersen February 12, 2020 08:42

haukepetersen requested changes Feb 13, 2020

View reviewed changes

haukepetersen mentioned this pull request Feb 13, 2020

xtimer: properly expose xtimer_usleep64() RIOT-OS/RIOT#13370

Merged

This was referenced Feb 13, 2020

xtimer: add xtimer_set_timeout_flag64() RIOT-OS/RIOT#13371

Merged

sys/event: add event_wait_timeout64() RIOT-OS/RIOT#13373

Merged

haukepetersen requested changes Feb 13, 2020

View reviewed changes

haukepetersen approved these changes Feb 14, 2020

View reviewed changes

bergzand added 2 commits February 14, 2020 10:57

npl/riot: Use RIOT conversion defines

2170af1

npl/riot: Prevent overflow issues in timer glue

2c8cee2

bergzand force-pushed the pr/npl_riot/timer branch from 7c6029b to 2c8cee2 Compare February 14, 2020 10:02

haukepetersen merged commit 310d6ac into apache:master Feb 14, 2020

bergzand deleted the pr/npl_riot/timer branch February 14, 2020 15:27

bergzand mentioned this pull request Feb 14, 2020

Hard faults bosmoment/PineTime-apps#11

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

npl/riot: Improve timer glue code #753

npl/riot: Improve timer glue code #753

bergzand commented Feb 10, 2020

sjanc Feb 11, 2020

bergzand Feb 11, 2020

sjanc Feb 12, 2020

haukepetersen commented Feb 13, 2020

haukepetersen left a comment

haukepetersen Feb 13, 2020

bergzand Feb 13, 2020

bergzand Feb 13, 2020

haukepetersen commented Feb 13, 2020

bergzand commented Feb 13, 2020

haukepetersen commented Feb 13, 2020

bergzand commented Feb 13, 2020 •

edited

Loading

bergzand commented Feb 13, 2020

haukepetersen commented Feb 13, 2020

bergzand commented Feb 13, 2020

haukepetersen left a comment

haukepetersen Feb 13, 2020

haukepetersen Feb 13, 2020

bergzand Feb 13, 2020

haukepetersen Feb 13, 2020

bergzand Feb 13, 2020

haukepetersen left a comment

haukepetersen Feb 13, 2020

haukepetersen Feb 13, 2020

bergzand commented Feb 13, 2020

haukepetersen left a comment

bergzand commented Feb 14, 2020

haukepetersen commented Feb 14, 2020

bergzand commented Feb 14, 2020

haukepetersen commented Feb 14, 2020

bergzand commented Feb 14, 2020

haukepetersen commented Feb 14, 2020

npl/riot: Improve timer glue code #753

npl/riot: Improve timer glue code #753

Conversation

bergzand commented Feb 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haukepetersen commented Feb 13, 2020

haukepetersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haukepetersen commented Feb 13, 2020

bergzand commented Feb 13, 2020

haukepetersen commented Feb 13, 2020

bergzand commented Feb 13, 2020 • edited Loading

bergzand commented Feb 13, 2020

haukepetersen commented Feb 13, 2020

bergzand commented Feb 13, 2020

haukepetersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haukepetersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergzand commented Feb 13, 2020

haukepetersen left a comment

Choose a reason for hiding this comment

bergzand commented Feb 14, 2020

haukepetersen commented Feb 14, 2020

bergzand commented Feb 14, 2020

haukepetersen commented Feb 14, 2020

bergzand commented Feb 14, 2020

haukepetersen commented Feb 14, 2020

bergzand commented Feb 13, 2020 •

edited

Loading