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.

Sign up for GitHub

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

Jump to bottom

mrf24j40: Move flags from netdev to radio #9583

Closed
wants to merge 3 commits into from
Closed

mrf24j40: Move flags from netdev to radio #9583

wants to merge 3 commits into from

Conversation

bergzand
Copy link
Member

Contribution description

The flags from the ieee802154 struct are only used in the radio code, there is no advantage of having make sense to have them in the netdev struct if their meaning is different per radio driver.

Also fixes a bug where the NETOPT_AUTOACK doesn't work, but is dis/enabled with the NETOPT_ACK_REQ setting.

Issues/PRs references

Minor cleanup preparing for #7736

Similar PRs are required for the other radio drivers using flags.

@bergzand bergzand added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 17, 2018
@bergzand bergzand requested a review from PeterKietzmann July 17, 2018 09:59
@bergzand
Copy link
Member Author

This changes the default flags enabled for the radio (NETOPT_ACK_REQ is no longer enabled by default), I have to take a look at how to solve this.

smlng
smlng previously requested changes Jul 30, 2018
View reviewed changes
Copy link
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: is it possible/worth to safe a byte here?

Otherwise I'm fine with the changes!

@bergzand
Copy link
Member Author

bergzand commented Aug 3, 2018

question: is it possible/worth to safe a byte here?

Should be possible, but there are still some random "flags" set here and there in the driver. I have to investigate whether they actually trigger something or that they were useless from the start.

@PeterKietzmann
Copy link
Member

@bergzand any progress?

@bergzand bergzand force-pushed the pr/mrf24j40/flag_refactor branch from 5616a49 to bd7d9de Compare September 29, 2018 15:34
@bergzand
mrf24j40: Resolve NETOPT_AUTOACK issues
c4c2f7b 
The NETOPT_AUTOACK command was never properly implemented in the
mrf24j40 driver and internally in the driver conflicted with the
NETOPT_ACK_REQ setting.
@bergzand
mrf24j40: Move flags from netdev to radio
b2b6a27 
The flags from the ieee802154 struct are only used in the radio code.
there is no advantage of having them in the netdev_ieee802154_t struct
if the flags are defined differently per radio driver.
@bergzand
fixup! mrf24j40: Move flags from netdev to radio
fc81331
@bergzand bergzand force-pushed the pr/mrf24j40/flag_refactor branch from bd7d9de to fc81331 Compare September 29, 2018 15:37
@bergzand
Copy link
Member Author

Should be possible, but there are still some random "flags" set here and there in the driver. I have to investigate whether they actually trigger something or that they were useless from the start.

@smlng: Looked around and had to remove two calls that set obsolete flags. The driver should still work (as far as I've tested). Not sure if it was the case before, but the ack_req setting is not set at boot with this PR.

PR is also rebased to latest master.

@PeterKietzmann
Copy link
Member

but the ack_req setting is not set at boot with this PR

But this is not intended, right?

@bergzand
Copy link
Member Author

bergzand commented Oct 4, 2018

But this is not intended, right?

That depends on whether you consider the old implementation/behaviour a bug or a feature :)

@PeterKietzmann
Copy link
Member

I just wanted to agree that ACKREQ should be enabled by default, no matter about bugs, features or whatever. We agreed in #9957.
@smlng do you see your comments addressed? Then, please dismiss your change request!

@smlng smlng dismissed their stale review October 4, 2018 11:49

comments addressed, needs testing

@PeterKietzmann
Copy link
Member

@smlng thanks. Would you mind setting the "Reviewed:.." labels for what you did? I'll run tests a bit later

@smlng smlng added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Oct 4, 2018
@PeterKietzmann
Copy link
Member

PeterKietzmann commented Oct 4, 2018 •
edited
Loading

I've tested on a nucleo-l073rf + mrf24j40 (node A) and between that board and a samr21-xpro (node B).

ping6 1000 <ll node B> 1000 0 0`
1000 packets transmitted, 286 received, 72% packet loss, time 804.0634942 s

-> bad but 'know' issue

ifconfig 7 ack_req (on both)
ping6 1000 <ll node B> 1000 0 0`
1000 packets transmitted, 998 received, 1% packet loss, time 159.06552569 s

-> fine

ifconfig 7 -autoack (on node A)
ifconfig 7 ack_req (on node B)

-> as expexted: B requests ACKs, A doesn't send ACKs, B retransmits

ifconfig 7 set nid 0x12 (on both)

... and running 2. again -> as expected

I'm fine with this PR but it feels wrong to merge it, knowing that we lose default ACK_REQ. Let's wait with merging until the fix is around.

@bergzand bergzand mentioned this pull request Nov 13, 2018
Open
@PeterKietzmann
Copy link
Member

Could someone update me about the current status of the 802.15.4/netdev cleanups?

ack_req setting is not set at boot with this PR

Is this still the case?

@PeterKietzmann
Copy link
Member

@bergzand ping?

@PeterKietzmann
Copy link
Member

knowing that we lose default ACK_REQ.

@bergzand is the above still the case?

@bergzand
Copy link
Member Author

@jia200x Does this PR still make sense with your netdev plans?

@fjmolinas
Copy link
Contributor

@jia200x Does this PR still make sense with your netdev plans?

ping! @jia200x

@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 22, 2021
@MrKevinWeiss
Copy link
Contributor

Any update, should this be rebased or is it out of date now?

@jia200x
Copy link
Member

jia200x commented Jun 24, 2021

@jia200x Does this PR still make sense with your netdev plans?

I missed the comments! Sorry

This doesn't contradict the netdev plans and I'm not aware of anyone working on porting this radio to the radio HAL. So I would say it's fine to proceed.

@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@benpicco
Copy link
Contributor

benpicco commented Feb 2, 2023

mrf24j40 has been ported to the Radio HAL, I think we can close this now.

@mguetschow mguetschow closed this Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smlng smlng smlng left review comments

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

Assignees
No one assigned
Labels
Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@bergzand @PeterKietzmann @fjmolinas @MrKevinWeiss @jia200x @benpicco @smlng @mguetschow