-
Notifications
You must be signed in to change notification settings - Fork 108
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
Refactor to modular server/client/middleware architecture #44
Conversation
Wow, this is a crazy commit. Thanks for the contribution! If I understand the code correctly, this doesn't change any of the basic functionality, it is merely a different way of structuring and interacting with the existing code, along with some new features. Adds a number of features I (and others I'm sure) have really wanted, namely the OLED display, telnet client, logging of messages and connecting to a home network (haven't tested that yet, but I bet it's great for development). It moves the messy websocket code into a separate file, which has been on my list for a long time. I appreciate the use of header guards, something I've been meaning to add to LoRaLayer2. Really like the idea of using an IRC style console, pretty clever. There isn't a great reason to have a getopt style console, it's just what I'm most familiar with. Had some minor issues compiling with platformio. It wasn't able to find TinyGPS++.h. I had to add it to the lib_deps in I noticed that you have to uncomment the correct board in This points to an older commit of LoRaLayer2 and does not have the latest updates to chat app. I think this is due to a misunderstanding I addressed in sudomesh/LoRaLayer2#2. Those updates can easily be merged into this refactor. Currently, makeEspArduino is not supported. I was able to get it to compile by adding the necessary libraries, downgrading LoRaLayer2, adding the correct build_flag, and renaming main.cpp to main.ino. However, after compiling, it produces a massive quantity of linker errors, complaining about multiple definitions and undefined references, for a small snippet,
I started seeing these types of errors previously when I dabbled with using C++ code in this repo. It may be that makeEspArduino itself needs to be patched to allow it to link this code. I will look into it. When I first started writing this repo, the idea was to keep it as C-like as possible. I knew, when I refactored LL2 to use C++ classes, I was bending the rules to make more readable code. Given that we are working in an arduino environment, it is silly to fool ourselves into thinking that we aren't using C++. Ultimately, I wanted to head towards something like this, I doubt I would have done as good of a job. My main concerns are readability and support. This is certainly more comprehensible on a macro-level, in that it adds structure to something that was previously an amorphous blob at best and totally structure-less at worst. I have some difficulty understanding it on a lower-level, but that may be because it's essentially new code. I'm also not terribly well-versed in C++, as evidenced by my shoddy work on LoRaLayer2. As I read through the code and really start to use it, I'm sure it will begin to make sense. I would like to merge as much of this as possible. There are a few fixes and updates I need make before accepting the merge.
Once merged, we can work on switching from I'm very busy this week, so I may not get to working on this until the weekend. But I will try to post updates on my progress to this thread. Thanks again for the time + effort that clearly went into this PR! |
I was able to successfully compile and link this refactor with makeEspArduino. Those linker errors were mostly caused by missing libraries. The only annoying change is that main.cpp can't be named main.cpp. Apparently, that causes some strange conflict within makeEspArduino and claims an undefined reference to I opened an issue in the makeEspArduino repo, but I don't expect anything to come of it. @tlrobinson your options for renaming |
It's been fun!
Correct, unless I missed anything (the message ID is the only thing I can think of)
Sorry, I added that at the last minute, I'll make sure it's working on a clean checkout.
Yeah, though we could default to TTGO LoRa32 V2 since that's what most people seem to be using. BTW we could take inspiration (and even code, it's Apache licensed) from Paxcounter which supports a bunch of boards. Each one has it's own hardware abstraction layer file: https://github.com/cyberman54/ESP32-Paxcounter/tree/master/src/hal
Correct, I'll update it.
Personally I don't believe it's worth supporting two build systems, and I'm obviously partial to Platformio, but I'm happy to help you keep makeEspArduino working if needed. Maybe we could run both builds in CI.
It's probably possible to do a similar architecture in C, but C++ seemed more natural to me.
I can walk through a simplified example: DisasterRadio *radio = new DisasterRadio();
AsyncWebSocket ws_server("/ws");
void setup() {
// most of setup omitted
radio->connect(new LoRaClient());
WebSocketClient::startServer(&ws_server, [](WebSocketClient *ws_client) {
radio->connect(ws_client);
});
}
void loop()
{
radio->loop();
} This creates an instance of When In this case we're initially connecting a In the main If a client has a message it wants to send it calls That's basically it.
I'm also open to splitting this PR up if you prefer.
I could even do separate branches for history, console+serial+TCP, OLED, and GPS
Something like your |
Yes, someone else brought this to my attention. It looks promising. I'm in favor of adapting it to our needs. I have a Pycom LoPy4 sitting here I need to to test. I may try getting it to work with that.
I'm happy to keep supporting makeEspArduino, since I have continued to use that. I know the changes that need to be made to get it working and I will push them when I merge. I like the idea of doing some CI tests, has been on my "to do" list. Fyi, apparently, Thanks for the simplified example. That helped a lot. I'm pretty sure I have a grasp of how it works. I probably just need to dive in and write a new client to really learn. I don't see the point of splitting this in to separate PRs or branches, I would rather just keep everything in one place. Let me know if you plan on taking care of the LoRaLayer2 and message ID updates. Once those are done, I will fix the makeEspArduino builds and merge your commits. We should also work on documenting the operation of this refactor somewhere in the README or wiki. |
Happy to either provide both a Heltec V1 and a Heltec V2 board, or run tests on both/make accessible via SSH |
What's the correct syntax for platformio.ini? I'm trying this on Debian for the TTGO board. [env] ==== I get Error: Please setup environments in I also tried -D TTGO_LORA_V1 (with space) but that didn't work either. |
@samuk this is working for me,
Maybe try putting the build flag on the same line? That's the only obvious difference I see. |
Oh also I just noticed the |
That builds OK, but pio run -t uploadfs -t upload fails with
I can run pio run -t uploadfs successfully and afterwards pio run -t upload and they both complete. The OLED display shows MAC address, I see a Wifi Network and can load the webpage but it just displays 'disaster radio' and no app. Don't worry about trying to debug it, I can just wait for the release. |
Interesting. Sounds like an issue with flashing the web app. Try flash the firmware and fs from either master or from 0.1.1 release. And then re-flash just the firmware from the refactor. I'll try to reproduce that bug while working on this merge. |
Yeah nice that worked.. Thanks |
{ | ||
if (gps.location.isValid() && gps.location.age() < beacon_period) | ||
{ | ||
server->transmit(this, String("c|<" + username + "> ") + gps.location.lat() + ", " + gps.location.lng()); |
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.
Maybe this should be a different message type? g
or p
maybe. In case the another client wants to handle the information differently
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.
Yeah, I agree, and it should probably be a little more structured. The GPS support was really just a proof of concept.
Maybe there's some inspiration to be taken from ham radio's APRS https://en.wikipedia.org/wiki/Automatic_Packet_Reporting_System
uint16_t msg_id = 0; | ||
|
||
unsigned char buf[2 + message.length() + 2] = {'\0'}; | ||
memcpy(buf, &msg_id, 2); |
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.
This is unnecessary. The msg_id does not need to be added by the WebSocket Client, it is already included in the message by the transmitting node. This code is analogous to the sendToWS()
function from the original code, https://github.com/sudomesh/disaster-radio/blob/master/firmware/esp32_ttgo/main.ino#L88. Therefore it should only convert message
from a String
to an unsigned char
.
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.
After testing, I see that it doesn't work with out that phony msg_id
I think the reason is that in converting the received message (raw bytes) to a String
type you lose the fact that the msg_ID
is actually a two-byte binary value, instead it is represented by it's equivalent ASCII character. Thus, by adding the 0
character to the beginning, the String is now the correct length to be accepted by the web app.
Maybe there is some sort of work around by parsing the String and converting it back in binary, but the correct way to fix this, which was already discussed in the thread, is to pass around structs of char arrays (or pointers to char arrays) as the messages between the server and clients.
I will leave the current bandage fix in for now. But it should be fixed ASAP as it could cause unexpected side effects.
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.
You'll see LoRaClient
and WebSocketClient
are the only clients that strip out message ID before calling transmit()
and add in dummy message IDs in receive()
, since those were the only existing clients I had to port.
|
||
// secrets in a separate file so we don't accidentally commit credentials | ||
#if __has_include("secrets.h") | ||
#include "secrets.h" |
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.
I'm going to add an untracked secrets.h
file to the repo, with those commented out lines and a note that this is where you should add secret information.
} | ||
if (WiFi.status() == WL_CONNECTED) | ||
{ | ||
ip = WiFi.localIP(); |
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.
If it fails to connect to your home wifi network, it should set this vaiue to 192.168.4.1
, instead of the local 0.0.0.0
which is of no value to the user.
WiFi.setHostname(HOST_NAME); | ||
WiFi.softAP(ssid); | ||
|
||
ip = WiFi.localIP(); |
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.
I guess the above comment should be placed down here. Same goes, we should figure out a way to dynamically retrieve the real IP address of the node in AP mode, maybe it could be just be set in config.h
? Or maybe if we create a preferences
file of some sort, that could then be reconfigured from the web gui or console?
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.
I reviewed all the files in this merge and left a number of comments. Nothing jumps out as a major problem. I have a commit ready that will address most of my concerns. Let me know if there is anything you desperately want to add before merging. I will wait until tomorrow night (1/26) to add my commits and merge.
memcpy(&msg_id, packet.data, 2); | ||
memcpy(msg, packet.data + 2, len - 2); | ||
|
||
server->transmit(this, String(msg)); |
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.
This message should not be converted to a string as it contains binary information and causes the msg_id
issue in the WebSocketClient. Will be left in on merge, but should be fixed ASAP.
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.
Yeah, I'm stripping out the first 2 bytes by doing + 2
in the second memcpy. As we discussed, I think changing "message" to be a struct containing the message ID (as uint16_t
?), message type (as char
?) and message payload (null terminated C string or bytes + length?) would be the right way to preserve the message ID.
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.
Are you saying the payload (excluding message ID) can contain binary data? In that case bytes+length rather than C string would be required.
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.
Yes, the payload can contain binary data. For example, the routiing table packets, which are sent from LoRaLayer2 to the WebSocket are entirely binary data
…s LoRaLayer2 to latest, re-renames main.cpp to main.ino
Thanks for making the changes and merging this! Sorry I didn't have a chance to make the changes myself. |
Hello, great work on this project!
I'm not sure if you'll want to take this PR as a whole, or take ideas from it, or not at all, but I thought I'd start the conversation here.
I was looking at what you had and found it difficult to add features, so I ended up refactoring it to be more modular. It's now essentially a library of components that can be plugged into each other in your
main.cpp
according to your application's requirements.Basically there are 3 main abstract classes, and a number of classes that inherit from them
(by "client" and "server" I mean they're running in the same process calling each other directly, not communicating over the network or anything)
DisasterServer
DisasterRadio
, acts as a sort of message bus sending messages from one "client" to all the otherstransmit
method called by clients (alsoconnect
/disconnect
/setup
/loop
)receive
methods on the clientsDisasterClient
WebSocketClient
instance for each client, and oneLoRaClient
for the LoRa radio (viaLoRaLayer2
receive
method called by server (alsosetup
/loop
)transmit
method on the serverDisasterMiddleware
which implements bothDisasterServer
andDisasterClient
transmit
andreceive
methods called by server and client it sits between (alsoconnect
/disconnect
/setup
/loop
)DisasterHistory
, a simple interface for classes that store a history of messagesrecord
(called byHistoryRecorder
client) andreplay
(called byHistoryReplay
middleware) methodsYou can connect clients to the server like this:
And you can chain calls to
connect
(sort of similar to Node.js'Stream
'spipe
method) to add functionality from middleware like this:(this sets up the serial port to be a client which uses the
WelcomeMessage
,HistoryReplay
andConsole
middleware)Here are the modules currently implemented:
DisasterServer
:DisasterRadio
: the main "server" that takes messages from clients and sends them to other clientsDisasterClient
:LoRaClient
: interfaces with LoRaLayer2WebSocketClient
: WebSocket connections from the web appStreamClient
: for ArduinoStream
, currently used for serial consoleTCPClient
: for a telnet-like serverHistoryRecord
: records messages to SD card (or bounded queue in memory)OLEDClient
: displays messages on the screen (mostly for debugging purposes, but eventually I'd like to use this for a mobile Disaster Radio terminal of some kind)GPSClient
: proof-of-concept, interfaces with a serial GPS module to beacon your current location periodicallyDisasterMiddleware
:WelcomeMessage
: (very simple, inmain.cpp
) shows a welcome message to clients when they connectHistoryReplay
: shows history to clients when they connectConsole
: implements a simple console with chat (similar to the web app) plus some/commands
(I went with a more IRC-like syntax but you could easily implement agetopt
version)DisasterHistory
:HistorySD
records history to SD cardHistoryMemory
records history to a bounded queue in memory (default limit 10 messages)Here's the complete "topology" I currently have in my
main.cpp
(but the default firmware would likely include less):Other features I've implemented in
main.cpp
:WIFI_SSID
andWIFI_PASSWORD
to try to connect to, and if it fails it falls back to AP mode (this is really useful for development so I don't have to keep switching my laptop's WiFi)Caveats / TODOs:
String
for messages because it was easy, but I'm aware of the pitfalls and it would probably be better to convert everything to C strings, buffers + length, or some message container objectLet me know what you think!