Chat app update, EspNow v2 & GPS Info #460
Conversation
📝 WalkthroughWalkthroughAdds an ESP‑NOW chat application and documentation. Introduces ChatApp with lifecycle and messaging APIs, ChatView (LVGL UI), ChatState (thread‑safe message storage), ChatProtocol (wire serialization/deserialization), and ChatSettings (persistence and optional encryption). Exposes ESP‑NOW payload limits and helper APIs (getVersion, getMaxDataLength) and adds EspNowService version tracking/getter. Implements ESP‑NOW enable/disable, send/receive flows, settings-driven restart, and channel/nickname handling. Adds GGA storage/getter in GpsService and expands GPS settings UI. All new code is conditionally compiled for ESP platforms with Wi‑Fi support. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Tactility/Source/service/gps/GpsService.cpp (1)
163-178: Potential race: GGA data could be lost immediately after receiving.The devices are started and callbacks subscribed at line 166. If a GGA sentence arrives before lines 169-170 execute, the valid
ggaTimewould be reset to 0, discarding the data.Consider resetting
rmcTimeandggaTimebefore starting the devices:Suggested reordering
+ rmcTime = 0; + ggaTime = 0; + for (auto& record: deviceRecords) { started_one_or_more |= startGpsDevice(record); } - rmcTime = 0; - ggaTime = 0; - if (started_one_or_more) {
🧹 Nitpick comments (12)
Tactility/Source/app/gpssettings/GpsSettings.cpp (1)
240-249: Consider clamping heading to valid range for defensive coding.While GPS course should always be 0-360°, if
headingwere negative, the expression(int)((heading + 22.5f) / 45.0f) % 8can yield a negative index in C++, causing out-of-bounds access ondirs[].♻️ Suggested defensive fix
float heading = minmea_tofloat(&rmc.course); if (!isnan(heading)) { + // Normalize heading to [0, 360) range + heading = fmodf(heading, 360.0f); + if (heading < 0) heading += 360.0f; const char* dirs[] = {"N", "NE", "E", "SE", "S", "SW", "W", "NW"}; // Calculate cardinal direction index (0-7) int idx = (int)((heading + 22.5f) / 45.0f) % 8;Documentation/chat.md (2)
20-31: Consider adding a language specifier to the fenced code block.The ASCII diagram code block could benefit from a language specifier like
textorplaintextto satisfy linting rules and improve rendering consistency.-``` +```text +------------------------------------------+ | [Back] Chat: `#general` [List] [Gear] |
52-76: Consider adding a language specifier to the wire protocol table.Similar to the UI layout diagram, this code block could use
textorplaintextas the language specifier.Tactility/Source/service/espnow/EspNow.cpp (1)
82-88: Consider logging when service is unavailable for consistency.Other functions in this file (e.g.,
enable,disable,addPeer) log an error when the service is not found. For consistency, you might consider adding a similar log here, though returning 0 as a fallback is a reasonable design choice.♻️ Optional: Add error logging for consistency
uint32_t getVersion() { auto service = findService(); if (service != nullptr) { return service->getVersion(); } + LOGGER.error("Service not found"); return 0; }Tactility/Source/service/espnow/EspNowService.cpp (1)
112-129: Consider resettingespnowVersionwhen disabling.The
espnowVersionmember is not reset to 0 indisableFromDispatcher(). While the version likely doesn't change between enable/disable cycles, resetting it would ensuregetVersion()returns 0 when the service is disabled, which might be more accurate.♻️ Optional: Reset version on disable
void EspNowService::disableFromDispatcher() { auto lock = mutex.asScopedLock(); lock.lock(); if (!enabled) { return; } if (esp_now_deinit() != ESP_OK) { LOGGER.error("esp_now_deinit() failed"); } if (!deinitWifi()) { LOGGER.error("deinitWifi() failed"); } + espnowVersion = 0; enabled = false; }Tactility/Source/app/chat/ChatSettings.cpp (2)
64-76: Consider usingsizeof(IV_SEED) - 1instead ofstrlen(IV_SEED).Since
IV_SEEDis a compile-time constant string literal, you could usesizeof(IV_SEED) - 1to avoid the runtime strlen call. This is a minor optimization.♻️ Suggested change
static bool encryptKey(const uint8_t key[ESP_NOW_KEY_LEN], std::string& hexOutput) { uint8_t iv[16]; - crypt::getIv(IV_SEED, strlen(IV_SEED), iv); + crypt::getIv(IV_SEED, sizeof(IV_SEED) - 1, iv);
88-89: Same optimization opportunity for consistency.Apply the same
sizeof(IV_SEED) - 1pattern here for consistency with the suggested change above.♻️ Suggested change
uint8_t iv[16]; - crypt::getIv(IV_SEED, strlen(IV_SEED), iv); + crypt::getIv(IV_SEED, sizeof(IV_SEED) - 1, iv);Documentation/espnow-v2.md (2)
28-31: Add language specifier to fenced code block.Per static analysis (markdownlint MD040), fenced code blocks should have a language specified. Use
textorlogfor log output.♻️ Suggested fix
-``` +```text I (15620) ESPNOW: espnow [version: 2.0] init I [EspNowService] ESP-NOW version: 2.0 ```
52-60: Add language specifier to fenced code block.Per static analysis (markdownlint MD040), fenced code blocks should have a language specified. Use
textfor the wire protocol format table.♻️ Suggested fix
-``` +```text Offset Size Field ------ ---- ----- 0 4 header (magic: 0x31544354) 4 1 protocol_version (0x01) 5 24 sender_name 29 24 target 53 1-1417 message (variable length) ```Tactility/Source/app/chat/ChatApp.cpp (1)
121-166: Consider simplifying hex validation withstd::isxdigit.The manual hex character validation works correctly, but could be simplified:
♻️ Optional simplification using std::isxdigit
// Parse hex key if (keyHex.size() == ESP_NOW_KEY_LEN * 2) { - bool validHex = true; - for (char c : keyHex) { - if (!((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F'))) { - validHex = false; - break; - } - } + bool validHex = std::all_of(keyHex.begin(), keyHex.end(), + [](unsigned char c) { return std::isxdigit(c); }); if (validHex) {This requires
#include <cctype>and#include <algorithm>(algorithm is already included).Tactility/Source/app/chat/ChatView.cpp (2)
254-267: Consider adding visual feedback for invalid/empty nickname.The save handler silently ignores empty nicknames. Users might be confused if clicking Save has no effect. Consider showing a brief message or preventing the save button from being enabled when the nickname is empty.
279-286: Empty channel input is silently ignored.Similar to settings save, if the user clears the channel field and clicks OK, nothing happens. Consider either:
- Defaulting to a fallback channel (e.g.,
#general)- Providing visual feedback that a channel name is required
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
Tactility/Source/service/espnow/EspNowService.cpp (1)
87-91: Consider explicitly initializingespnowVersionbefore the API call.If
esp_now_get_version()fails, theespnowVersionmember may retain an indeterminate value depending on how the ESP-IDF function handles the output parameter on failure. Setting it to 0 before the call ensures a known fallback.♻️ Suggested fix
+ espnowVersion = 0; if (esp_now_get_version(&espnowVersion) == ESP_OK) { LOGGER.info("ESP-NOW version: {}.0", espnowVersion); } else { LOGGER.warn("Failed to get ESP-NOW version"); }Tactility/Source/app/gpssettings/GpsSettings.cpp (1)
364-369: Consider adding a null check for the GPS service.If
findGpsService()returns nullptr, subsequent calls likeservice->getCoordinates()andservice->getGga()will cause a crash. While this may be an established pattern where the app assumes the service exists, a defensive check would improve robustness.♻️ Suggested defensive check
GpsSettingsApp() { timer = std::make_unique<Timer>(Timer::Type::Periodic, kernel::secondsToTicks(1), [this] { updateViews(); }); service = service::gps::findGpsService(); + tt_assert(service != nullptr, "GPS service not found"); }Documentation/chat.md (2)
3-3: Optional: Consider hyphenation for compound adjective.When "ESP-NOW based" modifies "chat application", it should be hyphenated as "ESP-NOW-based chat application" per standard English grammar rules for compound adjectives.
Based on static analysis hints.
📝 Suggested fix
-ESP-NOW based chat application with channel-based messaging. Devices with the same encryption key can communicate in real-time without requiring a WiFi access point or internet connection. +ESP-NOW-based chat application with channel-based messaging. Devices with the same encryption key can communicate in real-time without requiring a WiFi access point or internet connection.
129-130: Optional: Rephrase to improve readability.The last limitation item uses "only" twice in a way that makes the sentence feel cramped. Consider splitting into two separate bullet points for clarity.
Based on static analysis hints.
📝 Suggested improvement
-- All communication is broadcast; channel filtering is client-side only -- Messages > 250 bytes only received by devices running ESP-NOW v2.0 +- All communication is broadcast; channel filtering is client-side only +- Messages > 250 bytes are only received by devices running ESP-NOW v2.0Tactility/Source/app/chat/ChatApp.cpp (1)
163-173: Clamp channel length before persisting.An oversized channel will later trip
serializeMessage. Trim toTARGET_SIZE - 1before setting state/persisting.♻️ Proposed tweak
- state.setCurrentChannel(chatChannel); - settings.chatChannel = chatChannel; + const auto trimmedChannel = chatChannel.substr(0, TARGET_SIZE - 1); + state.setCurrentChannel(trimmedChannel); + settings.chatChannel = trimmedChannel;
| char message[MESSAGE_SIZE]; | ||
| }; | ||
|
|
||
| static_assert(sizeof(Message) == 1470, "Message struct must be exactly ESP-NOW v2.0 max payload"); |
There was a problem hiding this comment.
I would avoid making all messages this large. If you receive 10 messages, it's almost 15kB of memory.
It would also mean a lot of radio activity if you just want to send "Hi!".
I looked at this for inspiration:
https://www.localmesh.nl/en/meshcore-message-formats-payloads/
Total package size is maximum 256 bytes, but the package can be smaller!
Pernhaps we should also break it up into a header + a dynamically sized message part.
For example:
struct __attribute__((packed)) MessageHeader {
uint16_t protocol_version; // default to 1, which is current protocol version
uint32_t from; // sender id (randomly generated and stored in NVS or settings?)
uint32_t to; // receiver id, 0 for broadcast (e.g. when sending to all or sending to channel)
uint8_t payload_type;
bytes payload_size;
}
In case payload_type is 1, we assume it's a text message:
The remaining bytes would contain the sender's nickname and the message they sent:
- read bytes until you read nullptr. That's the nickname.
- read the remaining bytes as the text message.
There was a problem hiding this comment.
If this is a bit too much then feel free to just reduce the message size for now to 256 bytes or so, and I can look at it myself later.
KenVanHoeylandt
left a comment
KenVanHoeylandt
left a comment
There was a problem hiding this comment.
This is great! 👍
| 4. Extract sender name, target, and message as strings | ||
| 5. Store in message deque | ||
| 2. Validate packet: | ||
| - Minimum size: 18 bytes (16 header + 2 null terminators) |
There was a problem hiding this comment.
I'd clarify that a bit more and do a minor protocol update for the following:
The protocol allows for 16 byte messages, but for the TextMessage specifically, the minimum size should be 16 + minimum_nickname_size + null + minimum_target_size + minimum_message_size:
minimum_nickname_size: 2 bytes, as single letter names are not ok, but there are valid 2 letter namesminimum_target_size: Not sure. Could be 0 for broadcasting and non-zero for channel? It can also be a minimum of 1 where it's set toB(orb) for broadcasting and starts with#for channel.minimum_message: 1 byte, e.g. a response like "?" is valid.
If you don't feel like changing this, we can leave it as-is and I can pick it up later myself.
In that case, just clarify that the minimum message size for the protocol is 16 bytes and the effective minmimum message size (for TextMessage) is 18 bytes.
| // Validate input lengths | ||
| if (senderName.size() > MAX_NICKNAME_LEN || target.size() > MAX_TARGET_LEN) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
If you feel like updating the protocol: I would add a minimum length check for both parameters.
| out.resize(HEADER_SIZE + payloadSize); | ||
|
|
||
| // Build header | ||
| MessageHeader header; |
There was a problem hiding this comment.
I'd rather see this as
MessageHeader header = {
...
};That way, my editor warns me when a field is forgotten.
2 participants
Summary
This PR adds the new improved ESP-NOW chat application, introduces ESP-NOW v2.0 support with larger payloads and backwards compatibility, and improves the GPS info panel with additional data and UI refinements.
Chat App
Updates to the chat app with channel support and persistent configuration, allowing nearby devices to communicate without Wi-Fi infrastructure.
Key features:
#general,#team1)Messages are filtered client-side by channel, and short messages remain compatible with older ESP-NOW v1.0 devices.
ESP-NOW v2.0 Support
Introduces ESP-NOW v2.0 awareness to the core service layer and updates the Chat app to take advantage of larger payload sizes where supported.
Highlights:
Requires ESP-IDF v5.4+ for ESP-NOW v2.0 support.
GPS Info Panel Improvements
Enhances the GPS information panel by exposing more useful data and improving layout and visual clarity.
Changes include:
Summary by CodeRabbit
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.