lay groundwork for a possible future architecture #1571
Conversation
🤖 Pull request artifacts
|
|
I would suggest maybe using functionality based flags, NO_ESP32 has been imperfect and would be better if we knew if wifi or psram was the actual functionality blocker. |
|
The change of the Portduino/Telemetry define is fine, but please make sure the new define actually gets set when using the portduino target. Apart from that i second the remarks by @garthvh . Let's rather get rid of negative defines regarding the architecture and shift to positive defines. I have a similar changeset for RP2040 support that compiles (tm) but does not run (tm) yet. :-) |
@caveman99, is there an example that you can give where this is not the case? The addition of #define NO_TELEMETRY to ./variants/portduino/variant.h was specifically intended to do just that. The existing code doesn't have a consistent convention, but at least some variants use #define NO_x in their variant.h |
|
@garthvh and @caveman99, I'd be very much in favor of a switch from the existing practice of negative "opt-out" defines to positive "opt-in" defines, and will follow up with an additional commit. That will force more source code lines to change, but I see no way to avoid this. |
|
@majbthrd please disregard my comment about the telemetry define for portduino, i missed the change at the very bottom where you included it. |
|
Hmm... something may not be right with the Github build system. The three targets that fail there (rak4631, rak4631_eink, and t-echo) compile locally without issue. In the Github build, it is failing on a line of code (Screen.h:11) that is #ifdef-ed out for those and other NRF52 targets. Does anyone have any clues as to what is going on? Regardless, here's the description of the PR: The existing not-NO_ESP32, PORTDUINO, and intermittent use of NRF52_SERIES scheme was replaced with one of these architecture definitions: ARCH_ESP32 Instead of the existing opt-out choices of NO_SCREEN, NO_GPS, NO_WIRE and USE_SIM_RADIO, and opt-in HAS_WIFI, there are a collection of purely opt-in choices: HAS_WIFI The above defines use a non-zero value to indicate an opt-in. Since there was a "HAS_EINK" in the code that could be confused with the above (since it is a choice of display rather than opting-in for a display), it was replaced with a "USE_EINK" that is more consistent with other uses in the source code of a USE_x. Much of ./src/configuration.h and ./src/DebugConfiguration.h was moved out of those files and into one of several architecture.h files. An architecture.h unique to the architecture goes in the include directory for the architecture, i.e. ./src/esp32 or ./src/nrf52 or ./src/portduino. As you can see in configuration.h, there is a three step process: 1: variant.h allows opt-out of HAS_ options I also moved the long list of #define HW_VENDOR out of configuration.h and into the corresponding architecture.h. This hopefully makes things more modular and extensible for board variants and different target architectures. |
|
Executive summary: not all include paths in Meshtastic-device necessarily include configuration.h Since the existing code uses configuration.h defines to opt-out of features (rather than opt-in), the earlier code could forget to include configuration.h and not notice. |
|
Great work! |
|
This pull request has been mentioned on Meshtastic. There might be relevant details there: https://meshtastic.discourse.group/t/meshtastic-1-3-29-public-preview/6241/1 |
Executive summary: lay some groundwork for adding future architectures such as the STM32WL whilst making minimum changes to the existing source code
This PR is NOT to add the STM32WL; it just tweaks the way the build is configured so that individuals can more easily add architectures other than ESP32 and NRF52.
My own general plan to add an architecture is (spare time permitting) to do a USB-only port of Meshtastic-device to the STM32WL family. The initial firmware size (about half the flash available in the part) is much smaller than the over 300kByte image of the existing NRF52 port thanks to the absence of Bluetooth, USB stack, screen driver, and other assorted extras.
#defines added are:
NO_NRF (continuing the existing NO_ESP32 convention)
NO_BUTTON (continuing the existing convention of NO_SCREEN, NO_GPS, NO_WIRE, .etc) to omit code associated with pushbuttons, rotary controls, etc.
NO_TELEMETRY (this also follows the existing convention; it replaces existing use of PORTDUINO in ./src/modules/Modules.cpp, since omission of telemetry is not specific to PORTDUINO)
Here is the nickel tour:
./platform.ini & variants/portduino/platformio.ini : exclude the future stm32wl subdirectory for existing architectures
./src/configuration.h : make the existing "HAS_WIFI" define specific to ESP32 and PORTDUINO
main.cpp & ./src/modules/Modules.cpp : introduce NO_BUTTON to omit code associated with pushbuttons, rotary controls, etc.
./src/mesh/mesh-pb-constants.cpp : the functions readcb() and writecb() are called from only one file, and that is ./src/mesh/NodeDB.cpp. The caller code is inside an #ifdef FSCom; the same #ifdef condition is applied to functions readcb() and writecb() in /src/mesh/mesh-pb-constants.cpp
./src/modules/Modules.cpp & ./variants/portduino/variant.h : replace usage of "#ifndef PORTDUINO" with an "#ifndef NO_TELEMETRY" that can be utilized by architectures other than PORTDUINO.