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

Add RAK4631 Ethernet Gateway with working JSON output to MQTT #4661

Merged
merged 23 commits into from
Oct 8, 2024

Conversation

beegee-tokyo
Copy link
Contributor

Added new variant RAK4631 with MQTT JSON enabled to be able to build a MQTT JSON gateway using Ethernet with the RAK4631.
JSON on nRF52 works when using ArduinoJson library.

Fixes #2804
Makes #3286 unnecessary

Tested messages:

  • meshtastic_PortNum_TEXT_MESSAGE_APP
  • meshtastic_PortNum_TELEMETRY_APP
  • meshtastic_PortNum_NODEINFO_APP
  • meshtastic_PortNum_POSITION_APP

Untested messages (do not know how to generate them or do not have the hardware):

  • meshtastic_PortNum_WAYPOINT_APP
  • meshtastic_PortNum_NEIGHBORINFO_APP
  • meshtastic_PortNum_TRACEROUTE_APP
  • meshtastic_PortNum_DETECTION_SENSOR_APP
  • meshtastic_PortNum_REMOTE_HARDWARE_APP

platformio.ini Outdated Show resolved Hide resolved
@caveman99
Copy link
Member

Great work, do you have an insight how much bigger this makes the firmware? We didn't use ArduinoJson at the time we introduced support since it's a massive lib...

Also - if Json is not supported on nRF with the current lib, why make this an additional variant? can't we just add this to the regular 4631 target?

@beegee-tokyo
Copy link
Contributor Author

Hi @caveman99

I made it a separate variant as I am thinking that we (RAKwireless) might offer an nRF52 based Meshtastic Gateway with Ethernet.
When used as a gateway, JSON to MQTT is a wanted feature for nRF52, but requires to disable some functions (WiFi, PKI and Power FSM)
When used as an end node, JSON over MQTT is not needed, so the smaller code base can be used.

For the code size I compiled from the latest firmware repo:
"standard" RAK4631:

Building .pio\build\rak4631\firmware.hex
Advanced Memory Usage is available via "PlatformIO Home > Project Inspect"
Generating UF2 file
RAM:   [==        ]  22.5% (used 55988 bytes from 248832 bytes)
Flash: [==========]  97.4% (used 793536 bytes from 815104 bytes)

When building for gateway function with WiFi, PKI and Power FSM disabled:

Building .pio\build\rak4631_mqtt_json\firmware.hex
Checking size .pio\build\rak4631_mqtt_json\firmware.elf
Advanced Memory Usage is available via "PlatformIO Home > Project Inspect"
RAM:   [==        ]  22.7% (used 56372 bytes from 248832 bytes)
Flash: [==========]  97.9% (used 797604 bytes from 815104 bytes)

So not much difference. I think I can lower it by changing the MQTT::onReceive() function to use ArduinoJson as well. Then I can compile without JSON.cpp and JSONValue.cpp

For the tested message types, I set up an node with detection and meshtastic_PortNum_DETECTION_SENSOR_APP is working as expected.

@beegee-tokyo
Copy link
Contributor Author

Not sure about the name for the variant. rak4631_mqtt_json might not be the best. maybe it should be rak4631_gw.
What do you think?

@caveman99
Copy link
Member

compelling argument, i'd go with rak4631_eth_gateway for the variant name. Wanna hear what @thebentern thinks. Just one thought: PKI is required for remote admin; even if the gateway is ETH connected it might be aprt of a bigger centrally managed network. I'd leave this in if possible.

@beegee-tokyo
Copy link
Contributor Author

Agreed (after some digging what it is used for).
Removing

  • GPS
  • WiFi
  • Powermon
  • TZ
  • ExternalNotification
  • Paxcounter
  • RemoteHardware
  • StoreForward
  • CannedMessages
  • Waypoint
    Code size goes down to
Building .pio\build\rak4631_gw\firmware.hex
Advanced Memory Usage is available via "PlatformIO Home > Project Inspect"
RAM:   [==        ]  22.2% (used 55200 bytes from 248832 bytes)
Flash: [========= ]  91.9% (used 749260 bytes from 815104 bytes)

Any of these are required for a proper working stationary Ethernet gateway.

@beegee-tokyo
Copy link
Contributor Author

I usually prefer short names, like rak4631_gw, but mentioning the Ethernet functionality might be helpful.
Maybe rak4631_eth_gw

@caveman99
Copy link
Member

I usually prefer short names, like rak4631_gw, but mentioning the Ethernet functionality might be helpful. Maybe rak4631_eth_gw

Looks good. In case this product will get a RAK designator, we can also go with this label instead. We've been doing it for the field tester ...

GUVWAF
GUVWAF previously requested changes Sep 9, 2024
src/mqtt/MQTT.cpp Outdated Show resolved Hide resolved
@GUVWAF
Copy link
Member

GUVWAF commented Sep 9, 2024

You can test the NEIGHBOR_INFO_APP by enabling the NeighborInfo module, TRACEROUTE_APP by doing a traceroute (via the node list in the Android/iOS app) and WAYPOINT_APP by sending a waypoint by long-pressing in the map of the apps.

A downside of using another library here is that, since the JSON output needs to be added manually every time, now it needs to be done (and tested) in two places with slightly different API.

@beegee-tokyo
Copy link
Contributor Author

You can test the NEIGHBOR_INFO_APP by enabling the NeighborInfo module, TRACEROUTE_APP by doing a traceroute (via the node list in the Android/iOS app) and WAYPOINT_APP by sending a waypoint by long-pressing in the map of the apps.

A downside of using another library here is that, since the JSON output needs to be added manually every time, now it needs to be done (and tested) in two places with slightly different API.

I will give it a try.

@beegee-tokyo
Copy link
Contributor Author

@GUVWAF
Fixed #ifndef and rename the variant

@beegee-tokyo
Copy link
Contributor Author

Tested
Traceroute works
Waypoint works
NeighborInfo works

@Talie5in
Copy link
Contributor

Talie5in commented Sep 12, 2024

@beegee-tokyo Thoughts on excluding SCREEN as well, the SCREEN lib can take up a decent space from my testing when i was working in a different fork for a different reason.

Expectation of a RAK MQTT Gateway needing a screen?

I think time functions should remain remain (Assuming that's what TZ is?), as with ETH we have the ability to poll a NTP server to provide valid time to the mesh.

@beegee-tokyo
Copy link
Contributor Author

@beegee-tokyo Thoughts on excluding SCREEN as well, the SCREEN lib can take up a decent space from my testing when i was working in a different fork for a different reason.

Expectation of a RAK MQTT Gateway needing a screen?

I think time functions should remain remain (Assuming that's what TZ is?), as with ETH we have the ability to poll a NTP server to provide valid time to the mesh.

Good idea for the SCREEN. But I had to change two more modules because they were just using the OLED without checking if it is enabled. That lead to compilation errors. (Sorry for my crappy double #ifdef's).

With disable SCREEN and enabled TZ the code size goes down to 84%:

Advanced Memory Usage is available via "PlatformIO Home > Project Inspect"
RAM:   [==        ]  21.8% (used 54220 bytes from 248832 bytes)
Flash: [========  ]  84.4% (used 687736 bytes from 815104 bytes)

Continue testing now. No crashes so far.

@beegee-tokyo
Copy link
Contributor Author

Preview of the device:
Meshtastic Starterkit with Ethernet & PoE
3D printed enclosure

USB is only connected for flashing and debug. Device is powered through the PoE:

image

@Talie5in
Copy link
Contributor

I'll happily do some testing on this if this is still sitting open over the weekend, I have one or both of the modules (ethernet or the POE, don't recall, need to check)

@beegee-tokyo beegee-tokyo requested a review from GUVWAF September 12, 2024 02:04
@beegee-tokyo
Copy link
Contributor Author

I'll happily do some testing on this if this is still sitting open over the weekend, I have one or both of the modules (ethernet or the POE, don't recall, need to check)

@Talie5in
Would be great if another tester jumps in.
It will work without the PoE if you supply through USB.

@beegee-tokyo
Copy link
Contributor Author

Someone please help with the code formatting.
This TRUNK check drives me crazy. I already installed the Trunk extension, it checked my code files and changed intention and other things. But the Trunk Check Runner still fails.

@GUVWAF
Copy link
Member

GUVWAF commented Sep 12, 2024

@beegee-tokyo I ran trunk fmt on your branch, but it seems I can’t push to it so I opened a PR to your fork.

Since this is isolated in its own variant, in general I’m fine with this, but I don’t think you can expect the JSON part to be maintained as it requires manual changes using a different API and can only be tested with specific hardware.

@beegee-tokyo
Copy link
Contributor Author

@GUVWAF

What is missing to merge this?

@beegee-tokyo
Copy link
Contributor Author

@GUVWAF
Thank you.

@caveman99 caveman99 self-assigned this Oct 8, 2024
@caveman99
Copy link
Member

caveman99 commented Oct 8, 2024

Someone please help with the code formatting. This TRUNK check drives me crazy. I already installed the Trunk extension, it checked my code files and changed intention and other things. But the Trunk Check Runner still fails.

If you are developing on windows, trunk does not contain all the linters we use. notably cppcheck is missing. I always do the trunk check in WSL.

Let me resolve the conflict now and get this merged in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: MQTT with JSON crashes on nRF52 boards
7 participants