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

ieee802154/submac: fix initialization code #16533

Merged
merged 5 commits into from
Aug 4, 2021
Merged

ieee802154/submac: fix initialization code #16533

merged 5 commits into from
Aug 4, 2021

Conversation

jia200x
Copy link
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
@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
drivers/netdev_ieee802154_submac/netdev_ieee802154_submac.c Outdated Show resolved Hide resolved
sys/net/link_layer/ieee802154/submac.c Outdated Show resolved Hide resolved
@jia200x
Copy link
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
@@ -382,6 +382,7 @@ int ieee802154_submac_init(ieee802154_submac_t *submac, const network_uint16_t *
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
Contributor

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
Member Author

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
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
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

drivers/netdev_ieee802154_submac/netdev_ieee802154_submac.c Outdated Show resolved Hide resolved
sys/net/link_layer/ieee802154/submac.c Outdated Show resolved Hide resolved
@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
Contributor

Murdock is not happy

@jia200x
Copy link
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
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
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
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 PeterKietzmann is a code owner

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