Feature/node config update #4

jamienz · 2024-02-08T03:46:02Z

Adds the ability for a Host to remotely configure a Node. When the Node is about to send a message, it sends a "challenge" message to the Host first and includes its firmware version, and the Host may respond with the details for a firmware update. This PR adds the Node's config version to the challenge request, so the Host has the option to return a new configuration to the Node as part of a challenge response.

The configuration is application-specific, e.g.

the length of time the Node should sleep
how many samples the Node should take of an environmental sensor before averaging them
how long the Node should ignore signals from a PIR sensor after it has been triggered

EspNowNetwork already supports "generic" firmware, that can be the same for multiple Nodes. This PR allows Nodes to run generic firmware, but to fetch a potentially-Node-specific config from the Host.

respond to a node challenge with a config update.

…wNetwork into feature/node-config-update

Johboh

Thanks for the contribution @jamienz.
I have had this in my mind/to do as well, but so far haven't had a real need for sending configurations. Its a good idea, and I think we should add it (given the comments I added).
Also, did you see my comments on #1 and #3?

Johboh · 2024-02-08T14:58:42Z

examples/arduino/host/Host.ino

@@ -42,7 +47,8 @@ EspNowHost::OnApplicationMessage _on_application_message = [](EspNowHost::Messag
                                                              const uint8_t *message) {
  // Callback for new application messages.
  auto id = message[0];
-  Serial.println("Got message from 0x" + String(metadata.mac_address) + " with ID " + id);
+  Serial.println("Got message from 0x" + String(metadata.mac_address) + " with ID " +
+                 id); // + " rssi=" + String(metadata.rssi));


Suggested change

id); // + " rssi=" + String(metadata.rssi));

id);

Unless you address and merge #3 first :).

Also remember to update the Esp-idf example versions.

The Esp-idf host example doesn't set up a WiFi connection, so isn't a working example. Is that intentional? Should I add in the code to connect it to an access point, or just add in the changes to support this new configuration feature?

Yes it is, as I wanted to keep the examples simple and that is more code involved setting up WiFi using ESP-IDF. I should have added a "setup WiFi here" comment to make it more complete, as I have here.

When I myself find examples, I appreciate when the examples are as minimal as possible, and doesn't contains extras that is not actually needed. The extras can be in dedicated examples describing these extra features. Of course, as you say, without WiFi the node is not fully functional (thus the comment).

As you are using the Ardunio library (and not necessarily have the ESP-IDF framework setup), you can skip modifying the esp-idf examples, I can adjust them afterwards.

Johboh · 2024-02-08T15:02:43Z

examples/arduino/node/Node.ino


 #define FIRMWARE_VERSION 90201

+


I suggest adding this the Configuration example as another example that demonstrate how configuration works. This to keep this node example to a minimum as Configuration is optional (and will make it easier for the reader).
Suggest node_with_configuration or around those lines.

node_with_configuration and host_with_configuration have been added for Arduino, and the original node and host examples left as-is.

Johboh · 2024-02-08T15:03:36Z

examples/arduino/node/Node.ino

+  }
+
+  uint32_t timestamp2 = millis();
+  _on_log(("elapsed " + std::to_string(timestamp2 - timestamp1) + "ms").c_str(), ESP_LOG_DEBUG);


Can you elaborate on why you added this?

This has been removed. When it comes to battery powered nodes, the execution time and boot time are something I measure and try to minimize, but it shouldn't have been left in.

Johboh · 2024-02-08T15:05:03Z

src/host/EspNowHost.h

@@ -73,6 +75,9 @@ class EspNowHost {
  typedef std::function<std::optional<FirmwareUpdate>(uint64_t mac_address, uint32_t firmware_version)>
      FirmwareUpdateAvailable;

+  typedef std::function<std::optional<EspNowConfigEnvelope>(uint64_t mac_address, uint8_t config_version)>


Missing documentation.

CPP beginner question: For these callbacks, is it correct that the memory allocation for the struct that is returned is done within the function? Off the heap? For the FirmwareUpdate, there isn't an example of what this looks like. Also, in EspNowHost.cpp the struct that is received from the callback isn't explicitly deleted. Is this taken care of some other way? Would it be better to pass into the callback a struct that the callback can optionally populate, and return an int that indicates if it did so or not?

The "callbacks" we are using here are just pure function calls, in contrast to other observer patterns where callbacks might run at a later point by the scheduler (e.g. using FreeRTOS or similar queues,event busses etc).
As we are returning the struct here by value, the struct and its content will be copied to the caller. The struct allocated in the lambda will be destroyed when the function goes out of scope, and the same will be true for the "received" struct on the caller side.

(and yes, I should a better "FirmwareUpdate" example).

This is how my firmware update implementation looks like in my host:

std::optional<EspNowHost::FirmwareUpdate> HostDriver::onFirmwareUpdate(uint64_t mac_address, uint32_t firmware_version) { auto optdev = _device_manager.deviceForMac(mac_address); if (!optdev) { return std::optional<EspNowHost::FirmwareUpdate>{std::nullopt}; } auto type = optdev->get().type(); auto name = optdev->get().name(); auto hardware = optdev->get().hardware(); auto firmware_mqtt_path = _mqtt_remote.clientId() + "/firmware/current/" + type + "/" + hardware + "/" + makeMqttPathCompatible(name); // Do we have a newer firmware version for this type? auto update_information = _firmware_checker.getUpdateUrl(firmware_version, type, hardware); if (update_information) { _mqtt_remote.publishMessage(firmware_mqtt_path, "Updating to " + std::to_string(update_information->version), true); EspNowHost::FirmwareUpdate firmware_update; strncpy(firmware_update.wifi_ssid, _wifi_ssid, sizeof(firmware_update.wifi_ssid)); strncpy(firmware_update.wifi_password, _wifi_password, sizeof(firmware_update.wifi_password)); strncpy(firmware_update.url, update_information->url.c_str(), sizeof(firmware_update.url)); strncpy(firmware_update.md5, update_information->md5.c_str(), sizeof(firmware_update.md5)); return std::optional<EspNowHost::FirmwareUpdate>{firmware_update}; } _mqtt_remote.publishMessage(firmware_mqtt_path, std::to_string(firmware_version), true); return std::optional<EspNowHost::FirmwareUpdate>{std::nullopt}; }

Johboh · 2024-02-08T15:05:14Z

src/host/EspNowHost.h

@@ -96,7 +101,7 @@ class EspNowHost {
   */
  EspNowHost(EspNowCrypt &crypt, WiFiInterface wifi_interface, OnNewMessage on_new_message,
             OnApplicationMessage on_application_message, FirmwareUpdateAvailable firwmare_update = {},
-             OnLog on_log = {});
+             ConfigUpdateAvailable config_update = {}, OnLog on_log = {});


Missing @param documentation.

@param docs added

Johboh · 2024-02-08T15:49:21Z

src/shared/esp-now-config.h

+ * A container for the application-specific config data that is held within the payload.
+ * The payload could be anything, like a string, or json, but is probably a struct for efficiency.
+ */
+struct EspNowConfigEnvelope {


See comment in esp-now-structs.h regarding EspNowConfigEnvelope

Johboh · 2024-02-08T16:04:54Z

src/shared/esp-now-structs.h

@@ -47,6 +49,7 @@ struct EspNowDiscoveryResponseV1 {
 struct EspNowChallengeRequestV1 {
  uint8_t id = MESSAGE_ID_CHALLENGE_REQUEST_V1;


As this is a breaking change (e.g. if nodes are updated they will not function until host has be updated, and vice versa), technically you should add this as a new version, e.g. EspNowChallengeRequestV2. This is why I have version to begin with :) (and then handle both versions in node and host impls). But, this might be overkill. If we just update all the nodes first (and can live with them non functioning until host is updated), then its OK. So lets keep as is.

Johboh · 2024-02-08T16:05:40Z

src/node/impl/EspNowNode.cpp

@@ -224,6 +231,8 @@ bool EspNowNode::sendMessage(void *message, size_t message_size, int16_t retries
  // The challenge we expect to get back in the challenge/firmware response.
  request.challenge_challenge = esp_random();
  request.firmware_version = _firmware_version;
+  request.config_version = _config_version;


Suggested change

request.config_version = _config_version;

request.config_version = _configuration_revision;

Johboh · 2024-02-08T16:05:59Z

src/node/impl/EspNowNode.cpp

@@ -224,6 +231,8 @@ bool EspNowNode::sendMessage(void *message, size_t message_size, int16_t retries
  // The challenge we expect to get back in the challenge/firmware response.
  request.challenge_challenge = esp_random();
  request.firmware_version = _firmware_version;
+  request.config_version = _config_version;
+  log("challenge firmare version=" + std::to_string(_firmware_version) + " config version=" + std::to_string(_config_version), ESP_LOG_DEBUG);


Suggested change

log("challenge firmare version=" + std::to_string(_firmware_version) + " config version=" + std::to_string(_config_version), ESP_LOG_DEBUG);

log("challenge firmare version=" + std::to_string(_firmware_version) + " configuration revision=" + std::to_string(_configuration_revision), ESP_LOG_DEBUG);

Johboh · 2024-02-08T16:06:26Z

src/node/EspNowNode.h

@@ -211,6 +211,7 @@ class EspNowNode {
  OnStatus _on_status;
  EspNowCrypt &_crypt;
  esp_netif_t *_netif_sta;
+  uint16_t _config_version;


Suggested change

uint16_t _config_version;

uint16_t _configuration_revision;

jamienz · 2024-02-10T08:18:13Z

Thanks for all the thorough feedback @Johboh , and also for your patience. As you will have deduced, c/cpp/embedded is not my native language.

Johboh · 2024-02-10T08:54:26Z

Thanks for all the thorough feedback @Johboh , and also for your patience. As you will have deduced, c/cpp/embedded is not my native language.

no worries, I appreciate the contribution and I'm glad if I can share some knowledge.

jamienz added 13 commits February 1, 2024 21:11

feat (WIP): node config update

3e54876

respond to a node challenge with a config update.

feat: node config update

29f705b

fix: formatting

9b48808

fix: includes

dd4f371

fix: comments

2a0eafd

feature: ardunio host config example

fd0081f

feat (WIP): node config update

23cf7eb

respond to a node challenge with a config update.

feat: node config update

29c6ba3

fix: formatting

02f696f

fix: includes

ce8240f

fix: comments

4a5582d

feature: ardunio host config example

3405e8c

Merge branch 'feature/node-config-update' of github.com:jamienz/EspNo…

2258559

…wNetwork into feature/node-config-update

Johboh reviewed Feb 8, 2024

View reviewed changes

jamienz added 3 commits February 13, 2024 17:15

fix: review feedback

58a8bd7

fix: review fixes

364ec21

fix: review fixes

4684ed3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/node config update #4

Feature/node config update #4

jamienz commented Feb 8, 2024

Johboh left a comment

Johboh Feb 8, 2024

jamienz Feb 10, 2024

Johboh Feb 10, 2024

Johboh Feb 8, 2024

jamienz Feb 13, 2024

Johboh Feb 8, 2024

jamienz Feb 13, 2024

Johboh Feb 8, 2024

jamienz Feb 10, 2024

Johboh Feb 10, 2024

Johboh Feb 8, 2024

jamienz Feb 13, 2024

Johboh Feb 8, 2024

Johboh Feb 8, 2024 •

edited

Loading

Johboh Feb 8, 2024

Johboh Feb 8, 2024

Johboh Feb 8, 2024

jamienz commented Feb 10, 2024

Johboh commented Feb 10, 2024

		@@ -47,6 +49,7 @@ struct EspNowDiscoveryResponseV1 {
		struct EspNowChallengeRequestV1 {
		uint8_t id = MESSAGE_ID_CHALLENGE_REQUEST_V1;

	request.config_version = _config_version;
	request.config_version = _configuration_revision;

	log("challenge firmare version=" + std::to_string(_firmware_version) + " config version=" + std::to_string(_config_version), ESP_LOG_DEBUG);
	log("challenge firmare version=" + std::to_string(_firmware_version) + " configuration revision=" + std::to_string(_configuration_revision), ESP_LOG_DEBUG);

Feature/node config update #4

Are you sure you want to change the base?

Feature/node config update #4

Conversation

jamienz commented Feb 8, 2024

Johboh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Johboh Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamienz commented Feb 10, 2024

Johboh commented Feb 10, 2024

Johboh Feb 8, 2024 •

edited

Loading