Skip to content

ieee802154/submac: fix initialization code#16533

Merged
jia200x merged 5 commits intoRIOT-OS:masterfrom
jia200x:pr/submac_fixes
Aug 4, 2021
Merged

ieee802154/submac: fix initialization code#16533
jia200x merged 5 commits intoRIOT-OS:masterfrom
jia200x:pr/submac_fixes

Conversation

@jia200x
Copy link
Copy Markdown
Member

@jia200x jia200x commented Jun 7, 2021

Contribution description

This PR does essentially 3 things:

  • Move some of the initialization code of netdev_ieee802154_submac from netdev_submac_ieee802154_init to the actual _init function of netdev_driver_t. This is required in order to make sure the device is initialized before setting netdev stuff.
  • Fix the default SubGHz page
  • Call confirm_set_trx_state after setting the state to RX_ON during initialization. In some cases, the radio was not ready to do a state transition because request_set_trx_state didn't finish.

Testing procedure

  • Ensure hal dependent radios (cc2538, nrf52840) still work with gnrc_networking and tests/ieee802154_hal

Issues/PRs references

None so far

@jia200x jia200x added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking Area: drivers Area: Device drivers labels Jun 7, 2021
@jia200x jia200x requested review from benpicco, maribu and miri64 June 7, 2021 12:04
@github-actions github-actions bot added the Area: sys Area: System label Jun 7, 2021
@benpicco benpicco requested a review from jeandudey June 7, 2021 12:06
jeandudey
jeandudey previously requested changes Jun 7, 2021
View reviewed changes
@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Jun 7, 2021

done!

This was referenced Jun 7, 2021
Merged
Draft
leandrolanzieri
leandrolanzieri reviewed Jun 15, 2021
View reviewed changes
sys/net/link_layer/ieee802154/submac.c Outdated
CONFIG_IEEE802154_CCA_THRESH_DEFAULT) >= 0);

ieee802154_radio_request_set_trx_state(dev, IEEE802154_TRX_STATE_RX_ON);
while(ieee802154_radio_confirm_set_trx_state(dev) == -EAGAIN) {};
Copy link
Copy Markdown
Contributor

@leandrolanzieri leandrolanzieri Jun 14, 2021

Choose a reason for hiding this comment

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

This comment also applies to all the previous radio HAL function calls: shouldn't we check for possible error return codes? Otherwise this function is always returning success.

Copy link
Copy Markdown
Member Author

@jia200x jia200x Jun 15, 2021

Choose a reason for hiding this comment

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

yes, this must be fixed. I will piggyback some changes

@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Jun 15, 2021

I added some commits that improve error checks.
I also added assertions in cases where it's not possible to recover. This would indicate faulty hardware or that something really got wrong.

@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021
benpicco
benpicco reviewed Jul 4, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@benpicco benpicco left a comment •
edited
Loading

Choose a reason for hiding this comment

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

looks good, just some style nits.
Please squash directly

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 12, 2021
@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Jul 12, 2021

Murdock is not happy

@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Jul 12, 2021

fixed! I also piggybacked another fix. I was calling one of the ieee802154_xxx functions inside an assert. This wouldn't work if DEVELHELP is disabled

@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Aug 4, 2021

ping! I would like to move on with #16535, but this one is needed :)

@benpicco benpicco dismissed jeandudey’s stale review August 4, 2021 13:02

comments have been addressed

benpicco
benpicco approved these changes Aug 4, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Code looks good and both our sub-MAC drivers are still working.

@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Aug 4, 2021

thanks for the review!

@jia200x jia200x merged commit 39d27c2 into RIOT-OS:master Aug 4, 2021
@benpicco benpicco added this to the Release 2021.10 milestone Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leandrolanzieri leandrolanzieri leandrolanzieri left review comments

@benpicco benpicco benpicco approved these changes

@jeandudey jeandudey jeandudey left review comments

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

@maribu maribu Awaiting requested review from maribu

@cgundogan cgundogan Awaiting requested review from cgundogan

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

Assignees

No one assigned

Labels

Area: drivers Area: Device drivers Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2021.10

Development

Successfully merging this pull request may close these issues.

5 participants

@jia200x @benpicco @leandrolanzieri @jeandudey @MrKevinWeiss