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

drivers: ieee802154: rf2xx: Add missing handle_ack call #21773

Merged
merged 1 commit into from
Jan 29, 2020

Conversation

nandojve
Copy link
Member

@nandojve nandojve commented Jan 8, 2020

The rf2xx driver is doing automatic retransmissions in hardware based
on whether ACKs are received or not. Currently the driver is not
invoking ieee802154_radio_handle_ack() as other drivers are doing and
required by OpenThread since 4fe1da9. This add rf2xx_handle_ack method
to ensures required ACK processing when driver performs TX.

Fixes: #21763

Signed-off-by: Gerson Fernando Budke nandojve@gmail.com

drivers/ieee802154/ieee802154_rf2xx.c Outdated Show resolved Hide resolved
drivers/ieee802154/ieee802154_rf2xx.c Outdated Show resolved Hide resolved
drivers/ieee802154/ieee802154_rf2xx.c Outdated Show resolved Hide resolved
drivers/ieee802154/ieee802154_rf2xx.c Outdated Show resolved Hide resolved
drivers/ieee802154/ieee802154_rf2xx.c Show resolved Hide resolved
drivers/ieee802154/ieee802154_rf2xx.c Outdated Show resolved Hide resolved
The rf2xx driver is doing automatic retransmissions in hardware based
on whether ACKs are received or not. Currently the driver is not
invoking ieee802154_radio_handle_ack() as other drivers are doing and
required by OpenThread since 4fe1da9. This add rf2xx_handle_ack method
to ensures required ACK processing when driver performs TX.

Fixes: zephyrproject-rtos#21763

Signed-off-by: Gerson Fernando Budke <nandojve@gmail.com>
@nandojve nandojve changed the title drivers: ieee802154: rf2xx: Add missing handle_ack call [DNM] drivers: ieee802154: rf2xx: Add missing handle_ack call Jan 14, 2020
Copy link
Collaborator

@tbursztyka tbursztyka left a comment

Choose a reason for hiding this comment

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

try to rebase against master to get CI pass (the error does not seem to be related to your patch)

@jukkar
Copy link
Member

jukkar commented Jan 15, 2020

try to rebase against master to get CI pass (the error does not seem to be related to your patch)

Easiest is just to re-trigger CI tests so closing and then re-opening the PR is enough.

@jukkar jukkar closed this Jan 15, 2020
@jukkar jukkar reopened this Jan 15, 2020
@nandojve
Copy link
Member Author

Hi Folks, I added DNM because I don't have a full setup to test OT yet, sorry about not comment about it. I'll start to create setup this weeek I hope test this ASAP. After applying @tbursztyka tips I believe solution is ready, but need test first. @markus-becker-tridonic-com could you describe your setup so I can reproduce my own to test OT? Any tip how to create one and configure is welcome. You can find me at zephyrproject.slack.com too.

PS.: I have 2 - PCA10056 that may help and one Wireshark Sniffer.

@jukkar jukkar added area: Networking DNM This PR should not be merged (Do Not Merge) labels Jan 15, 2020
@jukkar
Copy link
Member

jukkar commented Jan 15, 2020

Adding also DNM label. Remember to remove the label after the PR is ready to be merged.

@markus-becker-tridonic-com
Copy link
Contributor

@nandojve my test set-up is the Tridonic Thread BR based on mbed, but it should also work with a Nordic dev board BR based on OT: https://openthread.io/guides/border-router .

@jukkar
Copy link
Member

jukkar commented Jan 28, 2020

ping @nandojve / @markus-becker-tridonic-com Is this PR ready for merge?

@nandojve
Copy link
Member Author

ping @nandojve / @markus-becker-tridonic-com Is this PR ready for merge?

Hello @jukkar, sorry for my delay. @markus-becker-tridonic-com did tests and report success at #21763. With this we fixed the issue and I'm removing DNM from PR Label. It's ready to be merged!

Thank you everyone for helping with this!

@nandojve nandojve changed the title [DNM] drivers: ieee802154: rf2xx: Add missing handle_ack call drivers: ieee802154: rf2xx: Add missing handle_ack call Jan 28, 2020
@jukkar jukkar removed the DNM This PR should not be merged (Do Not Merge) label Jan 29, 2020
@jukkar jukkar merged commit 268f65d into zephyrproject-rtos:master Jan 29, 2020
@nandojve nandojve deleted the 21763 branch February 22, 2020 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

at86rf2xx radio driver does not report whether a TX was ACKed
4 participants