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

Feature/node config update #4

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
31 changes: 29 additions & 2 deletions examples/arduino/host/Host.ino
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#include "shared/esp-now-structs.h"
#include <Arduino.h>
#include <EspNowCrypt.h>
#include <EspNowHost.h>
Expand All @@ -14,6 +15,10 @@ struct MySecondApplicationMessage {
uint8_t id = 0x02;
double temperature;
};
struct MyConfigMessageV1 {
uint32_t sleep_seconds;
uint8_t foo;
};
#pragma pack(0)

// Change this to your LED pin
Expand Down Expand Up @@ -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));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Unless you address and merge #3 first :).

Also remember to update the Esp-idf example versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

switch (id) {
case 0x01: {
MyApplicationMessage *msg = (MyApplicationMessage *)message;
Expand All @@ -54,6 +60,9 @@ EspNowHost::OnApplicationMessage _on_application_message = [](EspNowHost::Messag
Serial.println("MyApplicationMessage::temperature: " + String(msg->temperature));
break;
}
default: {
Serial.println("Unknown message id: " + String(id));
}
}
};

Expand All @@ -62,6 +71,22 @@ EspNowHost::FirmwareUpdateAvailable _firmware_update_available = [](uint64_t mac
return std::nullopt;
};

EspNowHost::ConfigUpdateAvailable _config_update_available = [](uint64_t mac_address, uint16_t config_version) {
Serial.println(("_config_update_available start ver=" + std::to_string(config_version)).c_str());
if (config_version != 1) {
EspNowConfigEnvelope env;
env.version = 1;
MyConfigMessageV1 cfg;
cfg.sleep_seconds = 30;
cfg.foo = 111;
env.length = sizeof(MyConfigMessageV1);
memcpy(&env.payload, &cfg, env.length);
return (std::optional<EspNowConfigEnvelope>)env;
}

return (std::optional<EspNowConfigEnvelope>)std::nullopt;
};

EspNowHost::OnLog _on_log = [](const std::string message, const esp_log_level_t log_level) {
// Callback for logging. Can be omitted.
if (log_level == ESP_LOG_NONE) {
Expand Down Expand Up @@ -98,7 +123,7 @@ EspNowHost::OnLog _on_log = [](const std::string message, const esp_log_level_t

EspNowCrypt _esp_now_crypt(esp_now_encryption_key, esp_now_encryption_secret);
EspNowHost _esp_now_host(_esp_now_crypt, EspNowHost::WiFiInterface::STA, _on_new_message, _on_application_message,
_firmware_update_available, _on_log);
_firmware_update_available, _config_update_available, _on_log);

void setup() {
Serial.begin(115200);
Expand All @@ -114,6 +139,8 @@ void setup() {
Serial.println("have wifi");
Serial.print("IP number: ");
Serial.println(WiFi.localIP());
Serial.print("Channel: ");
Serial.println(WiFi.channel());

pinMode(LED_PIN, OUTPUT);
digitalWrite(LED_PIN, HIGH);
Expand Down
36 changes: 32 additions & 4 deletions examples/arduino/node/Node.ino
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@
#include <EspNowPreferences.h>
#include <esp_crt_bundle.h>

#define SLEEP_TIME_US (1000LL * 1000LL * 60LL * 1LL) // 1 minute
#define MICROSECONDS_PER_SECOND 1000000LL
#define SLEEP_TIME_US (MICROSECONDS_PER_SECOND * 60LL * 1LL) // 1 minute

#define FIRMWARE_VERSION 90201


Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// These structs are the application messages shared across the host and node device.
#pragma pack(1)
struct MyApplicationMessage {
Expand All @@ -18,6 +20,10 @@ struct MySecondApplicationMessage {
uint8_t id = 0x02;
double temperature;
};
struct MyConfigMessageV1 {
uint32_t sleep_seconds;
uint8_t foo;
};
#pragma pack(0)

// Encyption key used for our own packet encryption (GCM).
Expand Down Expand Up @@ -61,7 +67,6 @@ EspNowNode::OnLog _on_log = [](const std::string message, const esp_log_level_t
level = "unknown";
break;
}

Serial.println(("EspNowNode (" + level + "): " + message).c_str());
};

Expand Down Expand Up @@ -93,17 +98,40 @@ EspNowNode _esp_now_node(_esp_now_crypt, _esp_now_preferences, FIRMWARE_VERSION,

void setup() {
Serial.begin(115200);
uint32_t timestamp1 = millis();

_esp_now_preferences.initalizeNVS();

EspNowConfigEnvelope config_envelope;
MyConfigMessageV1 *cfg;
bool config_loaded = false;
if (_esp_now_preferences.getConfig(&config_envelope)) {
config_loaded = true;
cfg = (MyConfigMessageV1*) config_envelope.payload;
_on_log(("loaded sleep_seconds=" + std::to_string(cfg->sleep_seconds) + " foo=" + std::to_string(cfg->foo)).c_str(), ESP_LOG_DEBUG);
} else {
_on_log("no config loaded", ESP_LOG_INFO);
}

// Setup node, send message, and then go to sleep.
if (_esp_now_node.setup()) {

MySecondApplicationMessage message;
message.temperature = 25.6;
_esp_now_node.sendMessage(&message, sizeof(MySecondApplicationMessage));
} else {
_on_log("setup failed", ESP_LOG_ERROR);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on why you added this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

uint32_t sleep_time_us = SLEEP_TIME_US;
if (config_loaded) {
sleep_time_us = cfg->sleep_seconds * MICROSECONDS_PER_SECOND;
}

esp_deep_sleep(SLEEP_TIME_US);
_on_log(("sleeping for " + std::to_string(sleep_time_us / (MICROSECONDS_PER_SECOND)) + " seconds").c_str(), ESP_LOG_INFO);
esp_deep_sleep(sleep_time_us);
}

void loop() {}
void loop() {}
11 changes: 9 additions & 2 deletions src/host/EspNowHost.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
#include <optional>
#include <string>

struct EspNowConfigEnvelope;

/**
* @brief ESP Now Network: Host
*
Expand Down Expand Up @@ -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)>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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};
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment in esp-now-structs.h regarding EspNowConfigEnvelope, but here, similar to the FirmwareUpdate struct, add a struct ConfigurationAvailable {uint64_t new_revision, uint8_t length; const void *payload;}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
typedef std::function<std::optional<EspNowConfigEnvelope>(uint64_t mac_address, uint8_t config_version)>
typedef std::function<std::optional<EspNowConfigEnvelope>(uint64_t mac_address, uint32_t configuration_version)>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

ConfigUpdateAvailable;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ConfigUpdateAvailable;
ConfigurationAvailable;

I think we can drop the Update here and indicate that "a configuration is available" (same goes for all variables etc as well ofc)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed


enum class WiFiInterface {
AP, // Use the Access Point interface for ESP-NOW.
STA, // Use the Station/Client interface for ESP-NOW.
Expand All @@ -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 = {});
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing @param documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@param docs added


public:
/**
Expand All @@ -117,7 +122,8 @@ class EspNowHost {

void handleQueuedMessage(uint8_t *mac_addr, uint8_t *data);
void handleDiscoveryRequest(uint8_t *mac_addr, uint32_t discovery_challenge);
void handleChallengeRequest(uint8_t *mac_addr, uint32_t challenge_challenge, uint32_t firmware_version);
void handleChallengeRequest(uint8_t *mac_addr, uint32_t challenge_challenge, uint32_t firmware_version,
uint16_t config_version);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
uint16_t config_version);
uint64_t configuration_revision);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed


void sendMessageToTemporaryPeer(uint8_t *mac_addr, void *message, size_t length);

Expand All @@ -137,6 +143,7 @@ class EspNowHost {
OnLog _on_log;
OnNewMessage _on_new_message;
FirmwareUpdateAvailable _firwmare_update;
ConfigUpdateAvailable _config_update;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move up one line, and initialize _configuration_available before _firwmare_update om EspNowHost.cpp

OnApplicationMessage _on_application_message;
};

Expand Down
31 changes: 24 additions & 7 deletions src/host/impl/EspNowHost.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,10 @@ void EspNowHost::esp_now_on_data_callback(const esp_now_recv_info_t *esp_now_inf

EspNowHost::EspNowHost(EspNowCrypt &crypt, EspNowHost::WiFiInterface wifi_interface, OnNewMessage on_new_message,
OnApplicationMessage on_application_message, FirmwareUpdateAvailable firwmare_update,
OnLog on_log)
ConfigUpdateAvailable config_update, OnLog on_log)
: _crypt(crypt), _wifi_interface(wifi_interface), _on_log(on_log), _on_new_message(on_new_message),
_firwmare_update(firwmare_update), _on_application_message(on_application_message) {}
_firwmare_update(firwmare_update), _config_update(config_update),
_on_application_message(on_application_message) {}

void EspNowHost::newMessageTask(void *pvParameters) {
EspNowHost *_this = (EspNowHost *)pvParameters;
Expand Down Expand Up @@ -185,10 +186,11 @@ void EspNowHost::handleQueuedMessage(uint8_t *mac_addr, uint8_t *data) {
case MESSAGE_ID_CHALLENGE_REQUEST_V1: {
EspNowChallengeRequestV1 *message = (EspNowChallengeRequestV1 *)data;
auto firmware_version = message->firmware_version;
log("Got challenge request from 0x" + toHex(mac_address) +
", firmware version: " + std::to_string(firmware_version),
auto config_version = message->config_version;
log("Got challenge request from 0x" + toHex(mac_address) + ", firmware version: " +
std::to_string(firmware_version) + ", config version: " + std::to_string(config_version),
ESP_LOG_INFO);
handleChallengeRequest(mac_addr, message->challenge_challenge, firmware_version);
handleChallengeRequest(mac_addr, message->challenge_challenge, firmware_version, config_version);
break;
}

Expand All @@ -206,7 +208,8 @@ void EspNowHost::handleDiscoveryRequest(uint8_t *mac_addr, uint32_t discovery_ch
sendMessageToTemporaryPeer(mac_addr, &message, sizeof(EspNowDiscoveryResponseV1));
}

void EspNowHost::handleChallengeRequest(uint8_t *mac_addr, uint32_t challenge_challenge, uint32_t firmware_version) {
void EspNowHost::handleChallengeRequest(uint8_t *mac_addr, uint32_t challenge_challenge, uint32_t firmware_version,
uint16_t config_version) {
uint64_t mac_address = macToMac(mac_addr);

// Any firmware to update?
Expand All @@ -225,6 +228,20 @@ void EspNowHost::handleChallengeRequest(uint8_t *mac_addr, uint32_t challenge_ch
}
}

if (_config_update) {
log("checking for config_update", ESP_LOG_INFO);
auto metadata = _config_update(mac_address, config_version);
if (metadata) {
log("config update: version=" + std::to_string(metadata->version) + " len=" + std::to_string(metadata->length),
ESP_LOG_INFO);
EspNowChallengeConfigResponseV1 message;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment in esp-now-structs.h regarding EspNowConfigEnvelope, but this well result in the following here (untested):

auto challenge_response_size = sizeof(EspNowChallengeConfigurationResponseV1);
size_t full_message_length = challenge_response_size + metadata.length;
std::unique_ptr<uint8_t[]> full_message(new (std::nothrow) uint8_t[full_message_length]);
if (full_message == nullptr) {
    log("Failed to allocate configuration response message", ESP_LOG_ERROR);
    return;
}

auto *configuration_response = (EspNowChallengeConfigurationResponseV1*)full_message;
configuration_response->challenge_challenge = challenge_challenge;
configuration_response->revision = medata.new_revision;
configuration_response->length = medata.length;
memcpy(full_message + challenge_response_size , metadata.payload, metadata.length);
sendMessageToTemporaryPeer(mac_addr, full_message, full_message_length);

message.challenge_challenge = challenge_challenge;
memcpy(&message.envelope, &metadata, sizeof(EspNowConfigEnvelope));
sendMessageToTemporaryPeer(mac_addr, &message, sizeof(EspNowChallengeConfigResponseV1));
return;
}
}

// No firmware update (early return above)
EspNowChallengeResponseV1 message;
message.challenge_challenge = challenge_challenge;
Expand Down Expand Up @@ -294,4 +311,4 @@ std::string EspNowHost::toHex(uint64_t i) {
std::stringstream sstream;
sstream << std::hex << i;
return sstream.str();
}
}
1 change: 1 addition & 0 deletions src/node/EspNowNode.h
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,7 @@ class EspNowNode {
OnStatus _on_status;
EspNowCrypt &_crypt;
esp_netif_t *_netif_sta;
uint16_t _config_version;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
uint16_t _config_version;
uint16_t _configuration_revision;

uint32_t _firmware_version;
bool _setup_successful = false;
CrtBundleAttach _crt_bundle_attach;
Expand Down
11 changes: 11 additions & 0 deletions src/node/EspNowPreferences.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,17 @@ class EspNowPreferences : public EspNowNetwork::Preferences {
*/
bool espNowGetMacForHost(uint8_t *buffer) override;

/**
* @brief Set the config envelope that contains the application-specific config data.
*/
bool setConfig(EspNowConfigEnvelope *config_envelope) override;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is application specific and I don't think this should be part of Preferences/EspNowPreferences. Nodes that need uses configuration and needs to persist it, can do it themselves. The interface Preferences is added just for this reason so nodes can implement their own storage if they need to store more data which is common. They can do this by extending EspNowPreferences and add what they need, or store it separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the support for configuration as something that is built in, and handled as conveniently as possible for the Node, and likely used by a large percentage of Node implementations. The Node already has to create an instance of EspNowPreferences to pass to the EspNowNode constructor, and has to call the initalizeNVS() function, so having a convenience method for getting/setting the config on this class seems like a reasonable place for it.
Having said that, I'm happy to move it somewhere else.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jamienz We can include it in EspNowPreferences for convenience so the node can read and write the configuration using the same class (but as said earlier, I think its up to the node application to read and write, and not the EspNode.cpp implementation responsibly. It should just say "hey, there is a new revision, do what you want")


/**
* @brief Get the config envelope that contains the application-specific config data.
* @param config_envelope the EspNowConfigEnvelope to copy the data into
*/
bool getConfig(EspNowConfigEnvelope *config_envelope) override;

/**
* @brief After setting variables, call this to commit/save.
* @return true on success.
Expand Down
4 changes: 4 additions & 0 deletions src/node/Preferences.h
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#ifndef __ESP_NOW_I_PREFERENCES_H__
#define __ESP_NOW_I_PREFERENCES_H__

#include "esp-now-config.h"
#include <cstdint>

namespace EspNowNetwork {
Expand All @@ -26,6 +27,9 @@ class Preferences {
*/
virtual bool espNowGetMacForHost(uint8_t *buffer) = 0;

virtual bool getConfig(EspNowConfigEnvelope *config_envelope) = 0;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Se comment in EspNowPreferences

virtual bool setConfig(EspNowConfigEnvelope *config_envelope) = 0;

/**
* @brief Commit any changes written.
* @return true on success.
Expand Down
32 changes: 30 additions & 2 deletions src/node/impl/EspNowNode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ bool EspNowNode::setup() {
ESP_ERROR_CHECK(esp_wifi_set_storage(WIFI_STORAGE_RAM));
ESP_ERROR_CHECK(esp_wifi_set_mode(WIFI_MODE_STA));
ESP_ERROR_CHECK(esp_wifi_start());

// TODO(johboh): this might unset WIFI6 for ESP32-C6, but getting current protocols and appending WIFI_PROTOCOL_LR and
// then setting them again, fails with bad argument. Presumably a bug in esp_wifi_set_protocol not supporting
// WIFI_PROTOCOL_11AX?
Expand Down Expand Up @@ -150,6 +149,7 @@ bool EspNowNode::setup() {
if (_on_status) {
_on_status(Status::HOST_DISCOVERY_STARTED);
}

// Ok so we have no valid host MAC address.
// Announce our precence until we get a reply.

Expand Down Expand Up @@ -194,6 +194,13 @@ bool EspNowNode::setup() {
return false;
}

EspNowConfigEnvelope config_envelope;
if (_preferences.getConfig(&config_envelope)) {
_config_version = config_envelope.version;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pass configuration version in constructor as for firmware version.

} else {
_config_version = 0;
}

_setup_successful = success;
return success;
}
Expand Down Expand Up @@ -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;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);


// First, we must request the challenge to use.
bool got_challange = false;
Expand Down Expand Up @@ -267,6 +276,25 @@ bool EspNowNode::sendMessage(void *message, size_t message_size, int16_t retries
}
break;
}
case MESSAGE_ID_CHALLENGE_CONFIG_RESPONSE_V1: {
log("Got challenge update config response.", ESP_LOG_INFO);
EspNowChallengeConfigResponseV1 *response = (EspNowChallengeConfigResponseV1 *)decrypted_data.get();
// Validate the challenge for the challenge request/response pair
if (response->challenge_challenge == request.challenge_challenge) {
EspNowConfigEnvelope config_envelope;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here instead let the node have an optional callback with a uint32_t version, uint8_t length and void * payload, where the node can decide what to do on a new configuration. I think the configuration response should be a non breaking flow, e.g. it should be valid for the node to send messages after receiving a configuration response (so we set header.header_challenge = response->header_challenge;, got_challange = true; and call the callback here and not return)

std::memcpy(&config_envelope, &response->envelope, sizeof(EspNowConfigEnvelope));
_preferences.setConfig(&config_envelope);
_preferences.commit();
log("Saved config (version=" + std::to_string(config_envelope.version) + " ). Restarting.", ESP_LOG_INFO);
esp_restart();
} else {
log("Challenge mismatch for challenge request/ config response (expected: " +
std::to_string(request.challenge_challenge) +
", got: " + std::to_string(response->challenge_challenge) + ")",
ESP_LOG_WARN);
}
break;
}
}
}
}
Expand Down Expand Up @@ -421,4 +449,4 @@ void EspNowNode::handleFirmwareUpdate(char *wifi_ssid, char *wifi_password, char
}
vTaskDelay(1000 / portTICK_PERIOD_MS);
esp_restart();
}
}
Loading