markqvist / RNode_Firmware

RNode is an open, free and flexible digital radio interface with many uses
https://unsigned.io/rnode
GNU General Public License v3.0
145 stars 52 forks source link

Added PlatformIO support and fixed vulnerabilities #51

Closed attermann closed 7 months ago

attermann commented 7 months ago

Added platformio.ini with build profiles targeting all of the currentlty supported boards. Addressed some vulnerabilities exposed by the verbose build in PlatformIO, including:

attermann commented 7 months ago

@markqvist please see description of [ecf8792] for discussion of an issue with modem status flags. I don't have a good enough understanding of the code to know whether that's the correct fix so would appreciate your review.

markqvist commented 7 months ago

Hi @attermann, thanks a bunch for the PR! Some of these changes should be committed to a firmware release immediately. Some of them I am hesitant about, but please see the comments I wrote about that.

A few I am actually hesitant about on a conceptual level, including the addition of PlatformIO support. Not that I am against that in any way as such, but I do not use that system myself - at all. And I'm pretty sure it won't happen at any point too. This means I have no way to support, test and maintain it.

If it is included in the repo by default, people will expect it to work, and ask questions and raise issues if/when it doesn't. Realistically, I cannot maintain and support this, so I won't. Using PlatformIO instead of the current build system may also lead to various "incongruences for aesthetic reasons", where stuff is moved around purely to satisfy the compiler warning levels of somebody elses tastes.

Now, I know full well that this is NOT you, and that you know what you are doing, so I'm only saying this for perspective, but the last time someone tried to be helpful, coming from the perspective of compiler warnings, they submitted a completely untested PR, that absolutely entirely broke and destroyed the whole darned firmware. We can laugh about this now, but I only have so much time, and I really don't want to invite stuff like that - the firmware is deployed on thousands of devices, and needs a high level of functional assurance before being compiled and released.

So, bottom line is that I don't really now the best way forward. If we are to include PlatformIO support, somebody has to stand up and volunteer the responsibility to maintain it, and test it across all supported devices every time a new release is prepared. That's a bit much to ask, so I'm not gonna do it, but just leaving the possibility open, in case you feel like you have lots of time on your hands ;)

markqvist commented 7 months ago

Also, this PR needs a bit of discussing and adjusting here and there, but in about 48 hours, I will be seriously off the grid for a long time, and I really want to get your important fixes out and released in a firmware update before that happens.

For now I will manually merge those in, compile and test the firmware on devices and release it. Then there's amble time to figure out the best approach on the PlatformIO thing.

Again, thanks for the effort!

attermann commented 7 months ago

Some of your comments above are missing references to the actual change being discussed so I'm not sure how to respond. This may be because I re-forked your repo to re-organize my changes.

I have the latest after your cherry-picking and still have one issue for when you return.

/Users/chad/Documents/PlatformIO/RNode_Firmware_orig/RNode_Firmware.ino:447:7: error: 'update_csma_p' was not declared in this scope

Not sure why the compiler in Arduino CLI accepts this, but it seems my compiler is treating the function as pure-C and requiring that it be declared prior to use. That was the reason for my moving this function to precede where it's first called.