Skip to content
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

kw2xrf: CSMA support missing in driver #10364

Open
jnohlgard opened this issue Nov 12, 2018 · 13 comments
Open

kw2xrf: CSMA support missing in driver #10364

jnohlgard opened this issue Nov 12, 2018 · 13 comments
Assignees
Labels
Area: drivers Area: Device drivers Area: network Area: Networking Community: help wanted The contributors require help from other members of the community Platform: ARM Platform: This PR/issue effects ARM-based platforms State: don't stale State: Tell state-bot to ignore this issue Type: new feature The issue requests / The PR implemements a new feature for RIOT

Comments

@jnohlgard
Copy link
Member

Originally posted by @PeterKietzmann in #10355

Well, we're all short in time but we (HAW Hamburg) have certain interest in that radio so it might be us to do the follow-up. @gebart, @bergzand could you please open a feature request issue?

The kw2xrf driver does not support NETOPT_CSMA, but it does support NETOPT_AUTOCCA instead (and is currently the only driver which supports that flag)

The kw41z radio also does not support CSMA in hardware but only automatic CCA before TX (i.e. no automatic backoff and retry). The kw41zrf driver (#7107) implements CSMA exponential backoff and retry in software but utilizing the autocca hardware support. The same method could be used in the kw2xrf driver.

Whether the implementations can be shared between the two drivers or if it is better to reimplement the same behavior is left as an exercise to the reader.

@jnohlgard jnohlgard changed the title kw2xrf: Implement CSMA in driver kw2xrf: CSMA support missing in driver Nov 12, 2018
@jnohlgard jnohlgard added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: drivers Area: Device drivers Community: help wanted The contributors require help from other members of the community labels Nov 12, 2018
@jnohlgard
Copy link
Member Author

Some pointers towards the kw41zrf implementation:

TX initialization:
https://github.com/gebart/RIOT/blob/pr/kw41zrf/drivers/kw41zrf/kw41zrf_netdev.c#L100-L174

CCA feedback using the CCAIRQ flag:
https://github.com/gebart/RIOT/blob/pr/kw41zrf/drivers/kw41zrf/kw41zrf_netdev.c#L961-L1000

@stale
Copy link

stale bot commented Aug 10, 2019

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.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 10, 2019
@PeterKietzmann PeterKietzmann added the State: don't stale State: Tell state-bot to ignore this issue label Aug 19, 2019
@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Aug 19, 2019
@Ilmu011
Copy link

Ilmu011 commented Aug 26, 2019

I am working on this right now and there is one thing I don't understand;
https://github.com/gebart/RIOT/blob/pr/kw41zrf/drivers/kw41zrf/kw41zrf_netdev.c#L47-L51

#define KW41ZRF_CCA_TIME               8	//128 us  Ref Manual Page 2146
#define KW41ZRF_SHR_PHY_TIME          12	//192 us
#define KW41ZRF_PER_BYTE_TIME          2	//32 us
#define KW41ZRF_ACK_WAIT_TIME         54	//864 us
#define KW41ZRF_CSMA_UNIT_TIME        20	//320 us 

Where are these timing parameters coming from? I have perused the MKW41Z/31Z/21Z Reference Manual but were not able to find them except KW41ZRF_CCA_TIME, which is explicitly stated as 128 us.
https://www.nxp.com/files-static/32bit/doc/ref_manual/MKW41Z512RM.pdf#page=2146

Furthermore, why are these timing parameters even a thing? These parameters are currently used to start a timer, which is supposed to indicate whether the acknowledge frame in a TR-sequence has timed out. The timer is set BEFORE issuing the operation to hardware, using these timing parameters to estimate, when the TX operation will be complete and adding KW41ZRF_ACK_WAIT_TIME on top of that.
https://github.com/gebart/RIOT/blob/pr/kw41zrf/drivers/kw41zrf/kw41zrf_netdev.c#L125-L132

Why isn't the timer just set to KW41ZRF_ACK_WAIT_TIME AFTER the hardware has signaled us, that the TX operation is complete?

@PeterKietzmann
Copy link
Member

Where are these timing parameters coming from?

These parameters result from the 802.15.4 specification. For example: 802.15.4 has a data rate of 250kbit/s. How long does it take to transmit one bit? 1 bit / 250kbit/s = 4 us and (1bit x 8) / 250kbps/s = 32us for one byte respectively. How does this relate to #define KW41ZRF_PER_BYTE_TIME 2 //32 us? I assmue the timer that is used within that driver runs at 62.5 kHz. Then, one tick takes16us and two ticks take 32us as mentioned next to the define.

@PeterKietzmann
Copy link
Member

Furthermore, why are these timing parameters even a thing?

I don't understand. But in any way, please note that the driver that you are referring to is WIP and not part of the official RIOT master.

Why isn't the timer just set to KW41ZRF_ACK_WAIT_TIME AFTER the hardware has signaled us, that the TX operation is complete?

I'm not familiar with the implementation in that detail. Your concerns seem reasonable to me. Maybe @benemorius can shed light on the as he indicated to take over #7107.

@Ilmu011
Copy link

Ilmu011 commented Aug 26, 2019

I assmue the timer that is used within that driver runs at 62.5 kHz. Then, one tick takes16us and two ticks take 32us as mentioned next to the define.

That's how it is. I checked it before.

https://github.com/gebart/RIOT/blob/pr/kw41zrf/drivers/kw41zrf/kw41zrf.c#L177

@Ilmu011
Copy link

Ilmu011 commented Sep 3, 2019

Another thing; kw2xrf supports NETOPT_AUTOCCA and kw41zrf supports NETOPT_CSMA, despite both of them doing the exact same thing. Both use automatic CCA before TX operations, and in both cases that's exactly what the respective NETOPTs in both drivers enable. The only difference is, that kw41zrf additionally does exponential backoff.

CSMA emulation via usage of automatic CCA is exactly what NETOPT_AUTOCCA defines and should be supported here.

Perform channel clear assessment before transmitting

This may be a hardware feature of the given transceiver, or might be otherwise implemented in software. If the device supports CSMA this option will enable CSMA with a certain set of parameters to emulate the desired behaviour.

Actually, according to the documentation, it is even WRONG to support NETOPT_CSMA here;

If the device supports CSMA in hardware, this option enables it with default parameters. For further configuration refer to the other NETOPT_CSMA_* options.

Neither kw2xrf nor kw41zrf support CSMA in hardware. None should support NETOPT_CSMA, but should support NETOPT_AUTOCCA instead.

The advantage of supporting NETOPT_CSMA is that the driver can make use of the additional options for NETOPT_CSMA, like NETOPT_CSMA_RETRIES, NETOPT_CSMA_MAXBE or NETOPT_CSMA_MINBE, but from a specification standpoint, it's just wrong.

@Ilmu011
Copy link

Ilmu011 commented Sep 3, 2019

For efficiency's sake, for now I'll just throw out NETOPT_AUTOCCA from kw2xrf and implement support for NETOPT_CSMA, similar to kw41zrf, but something should be done about this in the future.

@benemorius
Copy link
Member

benemorius commented Sep 12, 2019

I was puzzled at first by the timer usage too. These radios have a tighter integration with software than others I've used. The use of the radio timer isn't just to fire callbacks in software. It's actually used to trigger events in the radio hardware like timing out an ACK wait for example.

In other words:

Why isn't the timer just set to KW41ZRF_ACK_WAIT_TIME AFTER the hardware has signaled us, that the TX operation is complete?

Because the hardware waits to signal us the outcome of the TX operation until it has received an ACK or not, and the timer is what causes it to stop waiting for a missed ACK and signal us. At least that's the case with kw41zrf. It looks like kw2xrf does as you say where the hardware signals TX complete before waiting for ACK and only then the driver starts an ACK timer. It's possible that both radios could support either paradigm and the difference in the two drivers is just developers' preference. You could save an otherwise unnecessary interrupt here.

My interpretation of NETOPT_AUTOCCA is that the clear channel assessment should be performed but NOT the retries. Meaning if CCA succeeds then the packet is transmitted but if it fails then the radio driver returns a medium-busy failure rather than retrying.

I interpret NETOPT_CSMA to mean NETOPT_AUTOCCA plus retries. And you're right the documentation specifically says that it must be hardware CSMA support which is interesting. Is that because we have (or intend to have (or used to indend to have)) software CSMA retries implemented in an upper layer? And maybe it's getting implemented in netdev drivers because the upper layer implementation is missing so far? I haven't looked in to that.

@benemorius
Copy link
Member

benemorius commented Sep 13, 2019

By the way there's a bug in the #7107 CSMA implementation that prevents any retransmissions from actually happening if the initial CCA fails.

After _isr_event_seq_t_ccairq() calls kw41zrf_tx_exec() to retransmit the last frame, _isr_event_seq_tr() cancels the transmission by calling kw41zrf_abort_sequence() while handling ZLL_IRQSTS_SEQIRQ_MASK.

But with that fixed I think it works as intended.

@Ilmu011
Copy link

Ilmu011 commented Oct 27, 2019

@PeterKietzmann
I opened #12589 regarding this issue.

@miri64
Copy link
Member

miri64 commented Jul 3, 2020

Should also be fixed by @jia200x lower-layer rework (see #13943)

@PeterKietzmann
Copy link
Member

The kinetis hardware provides dedicated timers for CSMA. Although a SubMAC might solve the issue already using AutoCCA, a CSMA driver is missing which might reduce load.

@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers Area: network Area: Networking Community: help wanted The contributors require help from other members of the community Platform: ARM Platform: This PR/issue effects ARM-based platforms State: don't stale State: Tell state-bot to ignore this issue Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

No branches or pull requests

6 participants