pkg/openthread: update for thread release 20180926 by Hyungsin · Pull Request #11204 · RIOT-OS/RIOT

Hyungsin · 2019-03-18T22:35:51Z

Contribution description

This PR brings OpenThread version released on 20180926 to RIOT-OS.

Miscellaneous fixes to match formats.
Add MicroTimer function for the future use (CSMA/CA at the OpenThread code).
Support data polling for sleepy end devices.
@jia200x / @biboc, please review and test this.

Testing procedure

I checked that two samr21-xpro boards can be connected and ping to each other.

Issues/PRs references

jia200x · 2019-03-19T10:03:50Z

awesome!

Hyungsin · 2019-03-20T21:00:07Z

Just tested on two IoT-LAB-M3 nodes :)

miri64 · 2019-07-30T17:11:36Z

@Hyungsin needs rebase. @jia200x can you give this another look?

jia200x · 2019-10-18T13:57:43Z

@Hyungsin my biggest apologies.

I just noticed I personally merged #11540 which also updates OpenThread to the same version.

However, this PR still covers some points that were not included in #11540 (e.g ACK packet handling)
Do you plan to continue working on this? If so, does it work for you if we add the implementation details of this PR in the current master?

Again, sorry for not checking the version :/

Hyungsin · 2019-11-15T19:00:44Z

@jia200x, @miri64, it is frustrating to rebase this PR because another PR #11540, which handles the same (actually smaller) issue but posted 2 months after this, was reviewed and merged first. Surprisingly, it took only two days for the PR #11540 to be merged, but this PR got zero review for the last 8 months.

@jia200x, I can rebase this but it would be good if you can test. Does it work for you?

miri64 · 2019-11-15T19:24:16Z

@jia200x, @miri64, it is frustrating to rebase this PR because another PR #11540, which handles the same issue but posted after this, was reviewed and merged first. This PR got zero reviews meanwhile.

Sorry, I wasn't aware about this happening. We are aware of this problem and try to be more careful in the future (see #12543). I hand the review to @jia200x.

Hyungsin · 2019-11-15T21:36:33Z

@jia200x @miri64 I rebased it. Hope that it is tested soon.

maribu

I sadly do not have the background to properly review this. But I checked everything that does not need knowledge with openthread to be reviewed like code style and general concepts. This might speed up thinks a bit when a reviewer with the correct background takes a look.

maribu · 2019-11-16T11:38:51Z


 static kernel_pid_t _pid;
 static otInstance *sInstance;
+static bool otTaskPending = false;


Unless external packages enforce some code style for compatibility, we should try to stick to the RIOT coding convention for a consistency. See https://github.com/RIOT-OS/RIOT/blob/master/CODING_CONVENTIONS.md#naming

we should update this in the file as well.
I would be ok to leave it as is in this PR and immediately after open another one for fixing this. What do you think?

maribu · 2019-11-16T12:14:22Z

+ * @param[in] aT0        The reference time.
+ * @param[in] aDt        The time delay in microseconds from @p aT0.
+ */
+void otPlatAlarmMicroStartAt(otInstance *aInstance, uint32_t aT0, uint32_t aDt)


I have never worked with openthread so far, so please forgive if I'm asking nonsense.

The implementation in this PR would allow one timer in total to be set with otPlatAlamMicroStartAt() and one with the milliseconds version. (And the version in master would allow one in total.) This seems to be strange to me.

If I had to guess, I would say one timer per otInstance would make most sense. And one timer in total would make second-most sense to. Can someone with the required knowledge comment on this?

I think it should be possible to only provide one common internal implementation and offer otPlatAlarmMicroStartAt() and otPlatAlarmMilliStartAt() as static inline wrappers on top if this in order to safe some ROM.

We need to call otPlatAlarmMilliFired(sInstance) when a millitimer is fired and otPlatAlarmMicroFired(sInstance) when a microtimer is fired. Without separation, I think it is hard to know what timer is fired. FYI, https://github.com/openthread/openthread/blob/master/src/core/common/timer.cpp

maribu · 2019-11-16T12:16:24Z

+}
+
+/* OpenThread will call this to stop alarms */
+void otPlatAlarmMicroStop(otInstance *aInstance)


This should also be possible to combine with the milli version (e.g. by one being a static inline wrapper for the other). Of course only if not two timers in total is actually what openthread expects.

maribu · 2019-11-16T12:17:15Z

+}
+
+/* OpenThread will call this for getting running time in microsecs */
+uint32_t otPlatAlarmMicroGetNow(void)


Providing this as static inline wrapper should safe some ROM.

This is called by OpenThread, the symbol has to be exposed. So I guess it couldn't be static inline :/

maribu · 2019-11-16T12:18:41Z


 static otRadioFrame sTransmitFrame;
 static otRadioFrame sReceiveFrame;
+static otRadioFrame sAckFrame;


Again this is in misalignment with RIOT's coding convention. (As are the one above, that are not related to this PR.) If there is not technical reason for this, this should be renamed in order to comply with the coding convention.

We have to adapt this file anyway in a further PR (lots of Camel Case here). Is it OK for you to leave it as is and we open a follow up right after we merge this?

maribu · 2019-11-16T12:20:15Z

+
 /* Called upon TX event */
-void send_pkt(otInstance *aInstance, netdev_t *dev, netdev_event_t event)
+void sent_pkt(otInstance *aInstance, netdev_t *dev, netdev_event_t event)


~~Why this change in name?~~
Please see comment in declaration.

maribu · 2019-11-16T12:22:51Z

        case NETDEV_EVENT_TX_COMPLETE_DATA_PENDING:
            DEBUG("openthread: NETDEV_EVENT_TX_COMPLETE_DATA_PENDING\n");
-            otPlatRadioTxDone(aInstance, &sTransmitFrame, NULL, OT_ERROR_NONE);
+            _create_fake_ack_frame(true);


This seems odd to me. Maybe it is only because of the naming and me not being familiar with openthread. But I would expect ACKs to be created upon receiving a frame, not after sending one.

Yes, this "fake" ack is created to report the result of a sent frame to the upper layer. This is not for responding to a received frame.

This same technique is used in some of the official platform implementations of Openthread.

maribu · 2019-11-16T12:25:53Z

+    uint8_t reversed_addr[IEEE802154_LONG_ADDRESS_LEN];
    for (unsigned i = 0; i < IEEE802154_LONG_ADDRESS_LEN; i++) {
-        reversed_addr[i] = (uint8_t) ((uint8_t *)aExtAddress)[IEEE802154_LONG_ADDRESS_LEN - 1 - i];
+        reversed_addr[i] = aExtAddress->m8[IEEE802154_LONG_ADDRESS_LEN - 1 - i];


This is network vs host byte order, right? If so, reversed_addr is a misleading name. (You could also take a look at the eui64_t type and see if it would be handy here.)

If the host byte order happens to be network byte order, this would not work. Maybe _set_long_addr() could be changed to always expect network byte order? (Or even eui64_t?)

OpenThread uses IEEE addresses in little endian (see OpenThread radio API)

So we need to reverse in order to write it in the hardware. Maybe we could choose a better name for this variable or add comments?

maribu · 2019-11-16T12:30:00Z

 * @param[in]  event              just occurred netdev event
 */
-void send_pkt(otInstance *aInstance, netdev_t *dev, netdev_event_t event);
+void sent_pkt(otInstance *aInstance, netdev_t *dev, netdev_event_t event);


OK. Now (in context of the API doc I get the rename). Maybe even changing it to pkt_sent()?

Yes. This is called after a packet transmission finishes. So may be pkt_sent is more appropriate.

maribu · 2019-11-16T12:32:15Z

 * @param[in]  dev                pointer to a netdev interface
 * @param[in]  tb                 pointer to the TX buffer designed for OpenThread
 * @param[in]  rb                 pointer to the RX buffer designed for Open_Thread
+ * @param[in]  ab                 pointer to the ACK buffer designed for Open_Thread


Shouldn't it be "designated" instead of "designed"? If so, would you mind to sneak in a fix of the doc of tb and rb (as separate commit) into this PR?

Either one makes sense to me. So I can change as you want.

jia200x · 2019-11-18T14:49:04Z

@jia200x, @miri64, it is frustrating to rebase this PR because another PR #11540, which handles the same (actually smaller) issue but posted 2 months after this, was reviewed and merged first. Surprisingly, it took only two days for the PR #11540 to be merged, but this PR got zero review for the last 8 months.

Hi @Hyungsin.
I totally get your frustration. My apologies, I was the one who merged #11540. There were not bad intentions, it was my mistake for not (re)checking existing solutions without approving and merging.

As @miri64 pointed out, we are aware of this situation and trying to fix it asap.

Thanks anyway that besides this, you rebased it to the current master. I will run the tests ASAP and we would be ready to go.

Hyungsin · 2019-11-19T04:12:23Z

I will run the tests ASAP and we would be ready to go.

It is OK and Thanks for testing!

Hyungsin · 2019-11-28T22:33:11Z

@jia200x ping?

jia200x · 2019-11-29T10:11:19Z

@jia200x ping?

I will now. I was busy with #11022

jia200x

Some stuff I found while reviewing

jia200x · 2019-12-03T12:54:46Z

+/* Interupt handler for OpenThread milli-timer event */
+static void _millitimer_cb(void* arg) {
+    (void)arg;
+    msg_t millitimer_msg;


This structure should be globally allocated. Otherwise the message will be lost.

jia200x · 2019-12-03T12:56:14Z

-        xtimer_set_msg(&ot_timer, dt, &ot_alarm_msg, thread_getpid());
-    }
+    uint32_t dt = aDt * US_PER_MS;
+    xtimer_set(&ot_millitimer, dt);


I would say it's better to use xtimer_set_msg here.

jia200x · 2019-12-03T12:56:57Z

+}
+
+/* Interupt handler for OpenThread micro-timer event */
+static void _microtimer_cb(void* arg) {


same as above. This can be replaced by xtimer_set_msg

jia200x · 2019-12-03T13:00:14Z

        case NETDEV_EVENT_TX_COMPLETE_DATA_PENDING:
            DEBUG("openthread: NETDEV_EVENT_TX_COMPLETE_DATA_PENDING\n");
-            otPlatRadioTxDone(aInstance, &sTransmitFrame, NULL, OT_ERROR_NONE);
+            _create_fake_ack_frame(true);


This same technique is used in some of the official platform implementations of Openthread.

Hyungsin · 2019-12-30T19:23:19Z

@jia200x just resolved your comments. Thanks!

aabadie · 2020-01-15T08:44:50Z

@jia200x, maybe you'll find some time to have another look here ?

jia200x · 2020-02-06T11:14:49Z

I'm back into this, I'm running some tests now.

jia200x · 2020-02-06T12:59:11Z

    DEBUG("otPlatRadioAddSrcMatchShortEntry\n");
    (void)aInstance;
    (void)aShortAddress;
+    bool pending = true;


This "fake" SrcAddressMatch table should do the trick (if a SED receives an ACK with FP, if there was no data for that address it will just timeout).

However, in order to be compliant with the OpenThread API, the otPlatRadioEnableSrcMatch should be implemented.
(see OpenThread radio API). Otherwise SED won't have a chance of receive a packet if SrcMatch is disabled

jia200x · 2020-02-06T13:01:04Z

+    uint8_t reversed_addr[IEEE802154_LONG_ADDRESS_LEN];
    for (unsigned i = 0; i < IEEE802154_LONG_ADDRESS_LEN; i++) {
-        reversed_addr[i] = (uint8_t) ((uint8_t *)aExtAddress)[IEEE802154_LONG_ADDRESS_LEN - 1 - i];
+        reversed_addr[i] = aExtAddress->m8[IEEE802154_LONG_ADDRESS_LEN - 1 - i];


OpenThread uses IEEE addresses in little endian (see OpenThread radio API)

So we need to reverse in order to write it in the hardware. Maybe we could choose a better name for this variable or add comments?

jia200x · 2020-02-06T13:06:48Z


 static otRadioFrame sTransmitFrame;
 static otRadioFrame sReceiveFrame;
+static otRadioFrame sAckFrame;


We have to adapt this file anyway in a further PR (lots of Camel Case here). Is it OK for you to leave it as is and we open a follow up right after we merge this?

jia200x · 2020-02-06T13:08:35Z

+}
+
+/* OpenThread will call this for getting running time in microsecs */
+uint32_t otPlatAlarmMicroGetNow(void)


This is called by OpenThread, the symbol has to be exposed. So I guess it couldn't be static inline :/

miri64 · 2020-06-25T09:56:25Z

@Hyungsin @jia200x any progress?

jia200x · 2020-09-16T13:18:47Z

ping @Hyungsin

benpicco · 2020-10-02T11:54:16Z

Should probably be updated to thread-reference-20191113 and the new Radio HAL.

jia200x · 2020-10-02T12:14:12Z

Should probably be updated to thread-reference-20191113 and the new Radio HAL.

Ideally.
OpenThread can run on top of the SubMAC (see #14979), so then we don't have to take care of some "high level" stuff (getting ACK frames and handing it in to the stack, etc).

stale · 2021-06-03T19:59:56Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

Hyungsin force-pushed the forupstream_openthread_2018_09_26 branch 3 times, most recently from a59746d to 74a3cb9 Compare March 18, 2019 23:02

miri64 requested a review from jia200x July 30, 2019 17:11

miri64 added Area: network Area: Networking Area: pkg Area: External package ports Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Jul 30, 2019

Hyungsin mentioned this pull request Nov 15, 2019

sys: xtimer concurrency/robustness improvement #9530

Merged

pkg/openthread: update for thread release 20180926

18cc126

Hyungsin force-pushed the forupstream_openthread_2018_09_26 branch from 74a3cb9 to 18cc126 Compare November 15, 2019 21:27

Hyungsin mentioned this pull request Nov 15, 2019

CONTRIBUTING/MAINTAINING: Clarify guidelines #12543

Merged

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 15, 2019

maribu reviewed Nov 16, 2019

View reviewed changes

jia200x requested changes Dec 3, 2019

View reviewed changes

squash: use xtimer_set_msg

02308fc

jia200x requested changes Feb 6, 2020

View reviewed changes

stale Bot added the State: stale State: The issue / PR has no activity for >185 days label Jun 3, 2021

stale Bot closed this Jul 8, 2021

Uh oh!

Conversation

Hyungsin commented Mar 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

jia200x commented Mar 19, 2019

Uh oh!

Hyungsin commented Mar 20, 2019

Uh oh!

miri64 commented Jul 30, 2019

Uh oh!

jia200x commented Oct 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hyungsin commented Nov 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Nov 15, 2019

Uh oh!

Hyungsin commented Nov 15, 2019

Uh oh!

maribu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jia200x commented Nov 18, 2019

Uh oh!

Hyungsin commented Nov 19, 2019

Uh oh!

Hyungsin commented Nov 28, 2019

Uh oh!

jia200x commented Nov 29, 2019

Uh oh!

jia200x left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hyungsin commented Mar 18, 2019 •

edited

Loading

jia200x commented Oct 18, 2019 •

edited

Loading

Hyungsin commented Nov 15, 2019 •

edited

Loading