stm32duino / STM32LoRaWAN

Arduino library to support LoRaWAN communication using the STM32WL series.
https://stm32duino.github.io/STM32LoRaWAN/
Other
40 stars 9 forks source link

Complete first version #3

Closed matthijskooijman closed 2 years ago

matthijskooijman commented 2 years ago

This version should be complete in terms of code and if it passes review, can be merged to main as far as I'm concerned (future fixes and improvements can be done on top of that).

I have not fixed the codespell issues yet, I'll add a commit for that later.

matthijskooijman commented 2 years ago

I pushed another version, turns out I forgot to push this morning before I went to bed, so this also includes the async examples and some other small changes I mentioned before. It also fixes all comments I resolved above, the unresolved ones are left for discussion.

I also included an astyle workflow, which automatically checks astyle (it's a bit simpler than the Arduino_Core_STM32 one, running just astyle without any python script. It does use the same .astylerc config, except I added keep-one-line-blocks to allow single-line inline method definitions for small methods.

I retroactively updated all commits to adhere to the coding style. For future reference, I did an interactive rebase and then replaced each pick xxx line with:

exec git checkout xxx .; astyle --project=.astylerc --recursive '*.c' '*.h' '*.ino'; exec git commit -a -C xxx

This essentially just re-applies each commit (but to prevent conflicts from cherry-picking subsequent commits after changing earlier ones, this just resets to the unmodified state everytime and re-applies all astyle changes all the time, using -C to just copy commit information from the original history).

fpistm commented 2 years ago

Hi @matthijskooijman

I've made a script to add pragma to avoid unused parameters warning here: https://github.com/stm32duino/STM32LoRaWAN/commit/4f7cfa5bd6909b77fc51573cfef4d8b7da79bc6f

I guess you can ad dit and apply it. Then we can see some other warnings we should also removed. Maybe creating patch and I will raise internally an issue to fix it properly in the official release.

fpistm commented 2 years ago

Hi @matthijskooijman I've synced the WL repo which now include the core debug fix and so updated the WL update.

matthijskooijman commented 2 years ago

I've made a script to add pragma to avoid unused parameters warning here:

Cool. I've slightly reworked the script and added it, and also applied it.

I've also fixed the remaining warnings in STM32CubeWL, maybe those commits can be forwarded to the lora team for upstream inclusion? Ideally the unused parameter warnings would also be fixed, but I suspect that might partly be something for upstream LoRaMac-node rather than STM32CubeWL (but I haven't checked).

I've also fixed your other comments.

For ease of re-review, I have used fixup commits instead of force-pushing, so this needs one more round of rebase to squash those back into the history before this PR can be merged.

fpistm commented 2 years ago

I missed that before but there is no keywords.txt file to allows syntax coloration. Several API are colored as they have common name (begin, write,...) even joinOTAA is colored, I guess due to other library installed. Anyway, I guess having one would be better and followed Arduino library specification.

Hi @matthijskooijman

I've added the keywords.txt based on the MKRWAN one.

matthijskooijman commented 2 years ago

Thanks for fixing those! Looks good to me too, yay! (You did forget to squash the one fixup commit, but that's ok ;-p)

fpistm commented 2 years ago

Thanks for fixing those! Looks good to me too, yay! (You did forget to squash the one fixup commit, but that's ok ;-p)

Yes I forgot the fixup. 😅