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

netdev_ieee802154: add radio capabilities #11473

Closed
wants to merge 6 commits into from

Conversation

jia200x
Copy link
Member

@jia200x jia200x commented Apr 30, 2019

Contribution description

Several radios include hardware acceleration for common MAC tasks (automatic ACK handling, automatic retransmissions, CSMA, automatic FCS, etc).
Since these are MAC tasks, the MAC layer needs a mechanism for discovering which tasks are already handled by the radio.

Here comes the concept of radio capabilities.

E.g:

/* ieee802154_mac */

void _send(...) {
    netdev->driver->send(...);
    if(!(netdev->caps & NETDEV_IEEE802154_CAPS_FRAME_RETRIES)) {
        /* The radio does not handle the frame retransmission procedure. Use software logic */
        retrans_procedure();
    }
}

I took the caps that I think make more sense for RIOT. I added a typedef for caps so we don't have to change the API if the flags grow (e.g uint16_t to uint32_t).

This tends to be a common pattern (check Linux IEEE802154 MAC and OpenThread caps)
I didn't handle these caps as NETOPT because it's assumed a radio can enable or disable a feature if it exists but not otherwise. E.g it's indeed possible to disable the automatic ACK procedure while in extended mode of the driver (at82rf2xx), but the radio is still capable of handling ACKs.

This is required in order to implement components of the IEEE802.15.4 MAC and add basic mode support for radios.

Testing procedure

  • Check the capabilities for each radio match the datasheet/implementation
  • make doc
  • Check if the defined capabilities make sense

Issues/PRs references

I will be updating this list on the run

#11150

@jia200x jia200x added Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Area: network Area: Networking labels Apr 30, 2019
@jia200x jia200x requested a review from leandrolanzieri April 30, 2019 18:47
@miri64
Copy link
Member

miri64 commented May 2, 2019

Mhhhh, since these are things that do not change at runtime: is there maybe a way how to figure this out at compile-time (ideally with keeping multi-device support and keeping code-duplication low ;-))?

@miri64
Copy link
Member

miri64 commented May 2, 2019

Mhhhh, since these are things that do not change at runtime: is there maybe a way how to figure this out at compile-time (ideally with keeping multi-device support and keeping code-duplication low ;-))?

Just tossing some ideas, but how about a link-layer specific "MAC driver" struct (names are just examples take from the flags, please don't consider them to be serious)?

typedef struct {
    int tx_checksum(netdev_ieee802154_t *dev, ...);
    int csma(netdev_ieee802154_t *dev, ...);
    /* ... */
} netdev_ieee802154_mac_ops_t;

A driver then can set the once it supports in hardware to NOP, and otherwise use some common MAC implementation.

@miri64
Copy link
Member

miri64 commented May 2, 2019

(question is: is this more memory saving than your proposal?)

@jia200x
Copy link
Member Author

jia200x commented May 3, 2019

A driver then can set the once it supports in hardware to NOP, and otherwise use some common MAC implementation.

Hmmm the only issue is the fact these functions (tx checksum, csma) are usually implemented in common code on top of the driver. So, 3 transceiver without hardware FCS calculation will need to either implement this tx_checksum(...) function or point it to a common ieee802154_mac_tx_checksum()`. Hence, I tend to think a flag is more lightweight.

Opinions?

@PeterKietzmann
Copy link
Member

The flag based indication for feature availability seems more intuitive to. The other way around would lead to many "NOP" calls on our devices that often implement many features in hardware, or am I mistaken? However, I don't think that this PR is very urgent to merge unless we don't have the MAC implementation that uses it, so we could IMO wait until then.

@jia200x
Copy link
Member Author

jia200x commented Aug 26, 2019

Since #12070 is out, this PR starts to block :). I'm adding ACK support on top of the radios, and I need to know if a radio supports automatic ACK or not.
Someone minds to give it a try? :) It should be very easy to review

@bergzand
Copy link
Member

At the moment I think the flag based approach is fine. However, I think it would be good to have a few (static inline) wrapper functions to check for these flags. If, in the future, we have to change to a more complex function-based approach only the wrapper functions needs to be adapted.

@bergzand
Copy link
Member

netdev->caps & NETDEV_IEEE802154_CAPS_FRAME_RETRIES

This would be something like:

netdev_has_caps(netdev, NETDEV_IEEE802154_CAPS_FRAME_RETRIES)

which would be implemented along the lines of:

static inline bool netdev_has_caps(netdev_t *netdev, netdev_flags_t flag)
{
    return netdev->caps & flag;
}

@jia200x
Copy link
Member Author

jia200x commented Aug 26, 2019

@bergzand I like the idea.

@jia200x jia200x force-pushed the pr/ieee802154_radio_caps branch from e799768 to 4210525 Compare August 26, 2019 11:19
@jia200x jia200x force-pushed the pr/ieee802154_radio_caps branch from 4210525 to 505cee4 Compare August 26, 2019 11:29
@jia200x
Copy link
Member Author

jia200x commented Aug 26, 2019

Added netdev_ieee802154_has_cap function, as requested by @bergzand

@stale
Copy link

stale bot commented Feb 27, 2020

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 Feb 27, 2020
@jia200x
Copy link
Member Author

jia200x commented Mar 11, 2020

This one is not needed for now, so I will close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: stale State: The issue / PR has no activity for >185 days
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants