Abstract UI overhaul - #3431
Abstract UI overhaul#3431ripplebiz wants to merge 5 commits into
Conversation
…er interface * UITask's are now a MyMesh task Listener
|
I've tested this PR with
Looks to be working as expected. Tested message queue counters incrementing/decrementing while connected/disconnected, and the existing discover UI to find nearby repeaters is still working. The |
| MyMesh(mesh::Radio &radio, mesh::RNG &rng, mesh::RTCClock &rtc, SimpleMeshTables &tables, DataStore& store, AbstractUITask* ui=NULL); | ||
| class Listener { | ||
| public: | ||
| virtual void onMessageRecv(const ContactInfo &from, uint8_t txt_type, uint32_t sender_timestamp, uint8_t path_len, const char* text) = 0; |
There was a problem hiding this comment.
This is a bit lossy for clients that want to do more with the messages than previewing. Could we also get mesh::Packet* and extra/extra_len here? With this interface, I think standaolne uis would still need to tap into mymesh.
| class Listener { | ||
| public: | ||
| virtual void onMessageRecv(const ContactInfo &from, uint8_t txt_type, uint32_t sender_timestamp, uint8_t path_len, const char* text) = 0; | ||
| virtual void onChannelMessageRecv(ChannelDetails& channel_details, uint8_t path_len, const char* text) = 0; |
There was a problem hiding this comment.
same comment for this one. Having the pkt would be useful
weebl2000
left a comment
weebl2000
left a comment
There was a problem hiding this comment.
Looks good - a nit here and there
|
|
||
| void UITask::onControlDataRecv(const mesh::Packet* packet) { | ||
| #if UI_DISCOVER_SCREEN | ||
| if (packet->payload_len >= 12 && (packet->payload[0] & 0xF0) == CTL_TYPE_NODE_DISCOVER_RESP) { |
There was a problem hiding this comment.
handleDiscoverResponse reads payload[6..13] — memcpy(d->pubkey_prefix, &packet->payload[6], 8) and lookupContactByPubKey(&packet->payload[6], 8) — so this needs payload_len >= 14, not >= 12. simple_repeater/MyMesh.cpp builds the prefix-only response as 6 + 8 = 14 bytes and validates >= 6 + PUB_KEY_SIZE on its own receive path.
With a truncated or hand-crafted 12-13 byte DISCOVER_RESP whose tag matches, the discovery screen lists an entry whose last 1-2 prefix bytes are stale data left in the shared payload buffer by the previous packet. Not memory-unsafe (still inside payload[MAX_PACKET_PAYLOAD]), just garbage shown to the user.
Worth noting the >= 12 was inherited from the old code, but the old check was dead — payload[0] & 0xF0 != CTL_TYPE_NODE_DISCOVER_RESP parses as payload[0] & 1 by C precedence, so the whole condition was broken. Nice catch adding the parens; this is the first release where the length guard actually runs.
| if (packet->payload_len >= 12 && (packet->payload[0] & 0xF0) == CTL_TYPE_NODE_DISCOVER_RESP) { | |
| if (packet->payload_len >= 14 && (packet->payload[0] & 0xF0) == CTL_TYPE_NODE_DISCOVER_RESP) { |
| disc_nodes_count = 0; | ||
| mesh::Packet* req = the_mesh.createControlData(cmd_bytes, sizeof(cmd_bytes)); | ||
| if (req) { | ||
| the_mesh.sendZeroHop(req); | ||
| discovery_req_time = millis(); | ||
| return true; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Resetting only once the packet is actually allocated keeps the old results on screen when the send fails.
| disc_nodes_count = 0; | |
| mesh::Packet* req = the_mesh.createControlData(cmd_bytes, sizeof(cmd_bytes)); | |
| if (req) { | |
| the_mesh.sendZeroHop(req); | |
| discovery_req_time = millis(); | |
| return true; | |
| } | |
| return false; | |
| mesh::Packet* req = the_mesh.createControlData(cmd_bytes, sizeof(cmd_bytes)); | |
| if (req) { | |
| disc_nodes_count = 0; | |
| the_mesh.sendZeroHop(req); | |
| discovery_req_time = millis(); | |
| return true; | |
| } | |
| return false; |
| virtual void onUnhandledResponse(const ContactInfo &from, uint32_t tag, const uint8_t* data, uint8_t len) { } | ||
| virtual void onTraceRecv(mesh::Packet *packet, uint32_t tag, uint32_t auth_code, uint8_t flags, | ||
| const uint8_t *path_snrs, const uint8_t *path_hashes, uint8_t path_len) { } | ||
| virtual void onRawDataRecv(mesh::Packet *packet) { } |
There was a problem hiding this comment.
Listener is a new public extension point aimed at third-party forks; a fork that heap-allocates its listener and deletes through a MyMesh::Listener* hits UB. Nothing in-tree breaks today (both UITask instances are file-scope globals), but one vtable slot removes the trap.
| virtual void onRawDataRecv(mesh::Packet *packet) { } | |
| virtual void onRawDataRecv(mesh::Packet *packet) { } | |
| virtual ~Listener() { } |
| memcpy(&out_frame[i], &data[4], len - 4); | ||
| i += (len - 4); | ||
| _serial->writeFrame(out_frame, i); | ||
| } else if (_listener && len > 4) { |
There was a problem hiding this comment.
| } else if (_listener && len > 4) { | |
| } else if (_listener && len >= 4) { |
| #ifndef UI_DISCOVER_SCREEN | ||
| #define UI_DISCOVER_SCREEN 1 | ||
| #endif |
There was a problem hiding this comment.
Add guard to make the migration fail loud if not set.
| #ifndef UI_DISCOVER_SCREEN | |
| #define UI_DISCOVER_SCREEN 1 | |
| #endif | |
| #ifdef UI_NO_DISCOVER_SCREEN | |
| #error "UI_NO_DISCOVER_SCREEN is obsolete - use -D UI_DISCOVER_SCREEN=0 instead" | |
| #endif | |
| #ifndef UI_DISCOVER_SCREEN | |
| #define UI_DISCOVER_SCREEN 1 | |
| #endif |
4 participants
The companion UI code has been getting slapped around for a long time, and has become a bit of a mess, so this PR is attempting to re-structure and cleanup some of the dependencies, and also start paving the way for UI forks which are trying to get more ambitious.
The main structural change is to remove ALL UI concepts from the
MyMeshclass. This involves the introduction of theMyMesh::Listenerinterface, so that an external component (eg. the UI) can register as a listener for various events like receiving a contact message, etc.AbstractUITaskhas been refactored to be aMyMesh::Listener, so all the UI implementations can just override various interface methods to manage more advanced in-firmware functionality. Have a look at the refactored DISCOVER feature in /ui-new on generally how other standalone UI features should be structured.This was an opt-out UI feature by defining the
UI_NO_DISCOVER_SCREEN, but that is now refactored toUI_DISCOVER_SCREEN(must be zero if you Don't want that feature).Towards Standalone UI
There is still a fair ways to go to properly abstracting things to support a standalone/companion hybrid firmware. There are a LOT of forks out there which already 'try' to do this, but are quite messy, and have to resort to various contortions primarily because: The companion firmware was NOT deigned to be for standalone devices.
This refactor is a step towards getting to this kind of hybrid firmware, but some things need to be pointed out, like Request/Response handling. The companion still has to manage handling requests/responses which are originated by the app and only IF unhandled, passes responses to the
onUnhandledResponse()Listener method. So, in your fork, if you want to introduce a standalone feature, like login, then you need to initiate the sending of the request using methods in theBaseChatMeshclass, and save the 32-bit 'tag' yourself, and check for that tag inonUnhandledResponse().If a fork wants to automatically respond the some Request packet (like how the companion responds to Telemetry requests), then you need to override the
onUnhandledRequest()method. This must place the response packet data payload in the 'reply' buffer, and then return the byte length of the response data (or 0 if not handled).