-
Notifications
You must be signed in to change notification settings - Fork 2k
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/sx1280: add driver for SX1280 transceiver v2 #18033
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thanks for this one! Looks much better with the pkg system.
Some initial comments:
- please use doxygen styled comments so
/* */
instead of//
. - add your copyright and authorship (maybe also Didier/Olivier/Nicolas where relevant), I should not be the single author, I only helped with the pkg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more comments, mostly everything I'm commenting are nitpicks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more things were pointed out by the static checks.
Lets give the CI a run, although there are still many pending things |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic PR, I have a few minor remarks but this is probably already beyond the quality that I can provide.
I will see if I have or can get some hardware to test it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Final round of review, still standing comments, bust mostly looks good.
- do we need
dio2_pin
anddio3_pin
they are unused, should we remove for now? - still unanswered https://github.com/RIOT-OS/RIOT/pull/18033/files#r861497182
I took the liberty yo push some changes to add Kconfig and fix a couple of nitpicks.
I'm going to rebase to remove the unwanted commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too last nitpicks, you can squash right away after they are applied :)
9e85399
to
5833beb
Compare
@aabadie @MrKevinWeiss do you still have comments? |
I will try to look at the changes sometime today. I have some different timezones now.
|
No blocking comments from my side. It looks good but I did not test it. |
Murdock only showed two failed builds which are unrelated:
|
@Aymeric-Brochier do you mind posting again some test-output, just for history to know nothing was broken through all the refactoring? |
I'll then trigger a fast build and ACK |
New tests :
I tried to test like if did not know anything :
TX/RX TX/RX default params : Terminal 1:
Terminal 2 with another card :
Other trace available but maybe it should be enough |
Please squash! Thanks for re-testing! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK!
f2934df
to
779e249
Compare
Congrats @Aymeric-Brochier thanks for sticking through and for all the refactoring from #17847, super cool to see this one in! |
Contribution description
Hello,
This is my first PR feel free to notify me if I am not doing it correctly
I join recently a project which will be using RIOT-OS and one of my task is to contribute to RIOT
This PR is centered about the driver : sx1280 and its adaptation with netdev
Testing procedure
My last test was done with 2 board esp32 WROOM 32
cd tests/driver_sx1280
I am coding from MAC OS and use docker to compile it
I need to add a variable PORT
someting like this :
esp32-wroom-32
make BOARD=esp32-wroom-32 BUILD_IN_DOCKER=1 flash PORT=$PORT term
Test : getter/setter and some TX/RX with 2 esp-wroom-32
Few simple tests with Getter/Setter
TX/RX on same channel same BW same coding rate same spreading factor (Default) OK
TX/RX on different channel (not seing messages after set another freq) OK
TX/RX on same channel same BW (seing messages after set same freq (Not the default one) and rx start again) OK
TX/RX on same channel diff BW (not seing messages after set another BW) OK
TX/RX on same channel same BW (seing messages after set same BW and rx start again) OK
TX/RX on different sf (not seing messages after set another sf) OK
TX/RX on different sf (seing messages after set same sf and rx start again) OK
TX/RX on different cr (seing messages) I do not know if it is normal since for other parameters it is required to be the same for the communications
Interesting points :
Add a readme
Avoid using the RAL completely
Random function : is it okay like I did it ?
Getter
Setter
TX/RX
Once we have a shell on both card we can use the shell cmd :
Issues/PRs references
This PR is not related to known issues but is more a new feature
Closes #17847