From e95097085deb8f7a1337b56b5fec5073aefbd2c9 Mon Sep 17 00:00:00 2001 From: simon Date: Mon, 18 May 2026 23:21:48 +0200 Subject: [PATCH] Add uart_cmd helpers to deduplicate UART command handlers. Centralize protobuf decode, response init/send, registration, and common nanopb encode callbacks; refactor existing cmd_* modules to use them. Co-authored-by: Cursor --- main/CMakeLists.txt | 1 + main/README.md | 5 ++- main/cmd_accel_deadzone.c | 71 +++++++++++------------------ main/cmd_client_info.c | 43 +++++------------- main/cmd_espnow_unicast_test.c | 67 +++++++++++---------------- main/cmd_version.c | 36 +++------------ main/uart_cmd.c | 82 ++++++++++++++++++++++++++++++++++ main/uart_cmd.h | 37 +++++++++++++++ 8 files changed, 193 insertions(+), 149 deletions(-) create mode 100644 main/uart_cmd.c create mode 100644 main/uart_cmd.h diff --git a/main/CMakeLists.txt b/main/CMakeLists.txt index 5f7b473..45f2e6f 100644 --- a/main/CMakeLists.txt +++ b/main/CMakeLists.txt @@ -13,6 +13,7 @@ idf_component_register( "led_ring.c" "uart.c" "uart_proto.c" + "uart_cmd.c" "cmd_handler.c" "cmd_version.c" "cmd_client_info.c" diff --git a/main/README.md b/main/README.md index 98ae74e..abe95d5 100644 --- a/main/README.md +++ b/main/README.md @@ -275,6 +275,7 @@ Target: ESP32-S3. Close serial monitor on the UART adapter port before running ` | `uart.c/h` | Framed UART RX/TX | | `uart_proto.c/h` | Encode/send `UartMessage` | | `cmd_handler.c/h` | Command queue and dispatch | +| `uart_cmd.c/h` | Shared UART decode/send helpers for handlers | | `cmd_version.c/h` | VERSION handler | | `cmd_client_info.c/h` | CLIENT_INFO handler | | `client_registry.c/h` | Registered slave table | @@ -288,8 +289,8 @@ Target: ESP32-S3. Close serial monitor on the UART adapter port before running ` ## Adding a new UART command 1. Add or extend messages in `uart_messages.proto` and regenerate nanopb. -2. Create `cmd_*.c` with a handler; register with `msg_register_handler(MessageType_…, handler)`. -3. Reply via `uart_send_uart_message()` where needed. +2. Create `cmd_*.c` with a handler; register with `uart_cmd_register(MessageType_…, handler)`. +3. Decode with `uart_cmd_decode()` / `UART_CMD_REQ()`; reply with `uart_cmd_init_response()` + `uart_cmd_send()`. 4. Extend `goTool` or another host client to send the matching frame. For ESP-NOW-driven PC updates later: map slave state to `ClientInfo` and send `CLIENT_INFO` over UART from the master. diff --git a/main/cmd_accel_deadzone.c b/main/cmd_accel_deadzone.c index 29f703b..9609b09 100644 --- a/main/cmd_accel_deadzone.c +++ b/main/cmd_accel_deadzone.c @@ -1,29 +1,22 @@ #include "bosch456.h" #include "client_registry.h" #include "cmd_accel_deadzone.h" -#include "cmd_handler.h" #include "esp_log.h" #include "esp_now_comm.h" -#include "pb_decode.h" -#include "uart_messages.pb.h" -#include "uart_proto.h" -#include +#include "uart_cmd.h" static const char *TAG = "[ACCEL_DZ]"; -static void send_response(uint32_t deadzone, uint32_t client_id, bool success, - uint32_t slaves_updated) { - alox_UartMessage response = alox_UartMessage_init_zero; - response.type = alox_MessageType_ACCEL_DEADZONE; - response.which_payload = alox_UartMessage_accel_deadzone_response_tag; +static void reply(uint32_t deadzone, uint32_t client_id, bool success, + uint32_t slaves_updated) { + alox_UartMessage response; + uart_cmd_init_response(&response, alox_MessageType_ACCEL_DEADZONE, + alox_UartMessage_accel_deadzone_response_tag); response.payload.accel_deadzone_response.deadzone = deadzone; response.payload.accel_deadzone_response.client_id = client_id; response.payload.accel_deadzone_response.success = success; response.payload.accel_deadzone_response.slaves_updated = slaves_updated; - - if (uart_send_uart_message(&response) != ESP_OK) { - ESP_LOGE(TAG, "failed to send response"); - } + uart_cmd_send(&response, TAG); } static esp_err_t push_deadzone_to_slave(const client_info_t *client, @@ -32,8 +25,7 @@ static esp_err_t push_deadzone_to_slave(const client_info_t *client, return ESP_ERR_INVALID_ARG; } - esp_err_t err = - client_registry_set_accel_deadzone(client->id, deadzone); + esp_err_t err = client_registry_set_accel_deadzone(client->id, deadzone); if (err != ESP_OK) { return err; } @@ -42,28 +34,20 @@ static esp_err_t push_deadzone_to_slave(const client_info_t *client, } static void handle_accel_deadzone(const uint8_t *data, size_t len) { - alox_UartMessage uart_msg = alox_UartMessage_init_zero; + alox_UartMessage uart_msg; alox_AccelDeadzoneRequest req = alox_AccelDeadzoneRequest_init_zero; - bool have_request = false; - if (len > 0) { - pb_istream_t stream = pb_istream_from_buffer(data, len); - if (!pb_decode(&stream, alox_UartMessage_fields, &uart_msg)) { - ESP_LOGW(TAG, "decode failed"); - send_response(BMA456_DEFAULT_ACCEL_DEADZONE, 0, false, 0); - return; - } - if (uart_msg.which_payload == - alox_UartMessage_accel_deadzone_request_tag) { - req = uart_msg.payload.accel_deadzone_request; - have_request = true; - } + if (uart_cmd_decode(data, len, &uart_msg) != ESP_OK) { + ESP_LOGW(TAG, "decode failed"); + reply(BMA456_DEFAULT_ACCEL_DEADZONE, 0, false, 0); + return; } - if (!have_request) { - req.write = false; - req.client_id = 0; - req.all_clients = false; + const alox_AccelDeadzoneRequest *req_ptr = UART_CMD_REQ( + &uart_msg, alox_UartMessage_accel_deadzone_request_tag, + accel_deadzone_request); + if (req_ptr != NULL) { + req = *req_ptr; } if (req.write) { @@ -88,7 +72,7 @@ static void handle_accel_deadzone(const uint8_t *data, size_t len) { ESP_LOGI(TAG, "set deadzone %lu via unicast to %u/%u slaves", (unsigned long)req.deadzone, (unsigned)sent, (unsigned)n); - send_response(req.deadzone, 0, sent > 0 || bma456_is_ready(), sent); + reply(req.deadzone, 0, sent > 0 || bma456_is_ready(), sent); return; } @@ -97,7 +81,7 @@ static void handle_accel_deadzone(const uint8_t *data, size_t len) { ESP_LOGI(TAG, "set local deadzone %lu (no ESP-NOW; use -client or -all " "for slaves)", (unsigned long)req.deadzone); - send_response(req.deadzone, 0, true, 0); + reply(req.deadzone, 0, true, 0); return; } @@ -105,30 +89,25 @@ static void handle_accel_deadzone(const uint8_t *data, size_t len) { if (client == NULL) { ESP_LOGW(TAG, "client id %lu not found", (unsigned long)req.client_id); - send_response(req.deadzone, req.client_id, false, 0); + reply(req.deadzone, req.client_id, false, 0); return; } esp_err_t err = push_deadzone_to_slave(client, req.deadzone); - send_response(req.deadzone, req.client_id, err == ESP_OK, err == ESP_OK); + reply(req.deadzone, req.client_id, err == ESP_OK, err == ESP_OK); return; } - /* Read */ if (req.all_clients || req.client_id == 0) { - uint32_t dz = bma456_get_accel_deadzone(); - send_response(dz, 0, true, 0); + reply(bma456_get_accel_deadzone(), 0, true, 0); return; } uint32_t dz = 0; esp_err_t err = client_registry_get_accel_deadzone(req.client_id, &dz); - send_response(dz, req.client_id, err == ESP_OK, 0); + reply(dz, req.client_id, err == ESP_OK, 0); } void cmd_accel_deadzone_register(void) { - if (msg_register_handler(alox_MessageType_ACCEL_DEADZONE, - handle_accel_deadzone) != ESP_OK) { - ESP_LOGE(TAG, "register failed"); - } + uart_cmd_register(alox_MessageType_ACCEL_DEADZONE, handle_accel_deadzone); } diff --git a/main/cmd_client_info.c b/main/cmd_client_info.c index c59f57b..fe21c74 100644 --- a/main/cmd_client_info.c +++ b/main/cmd_client_info.c @@ -1,27 +1,13 @@ #include "client_registry.h" #include "cmd_client_info.h" -#include "cmd_handler.h" #include "esp_log.h" #include "pb_encode.h" -#include "uart_messages.pb.h" -#include "uart_proto.h" +#include "uart_cmd.h" static const char *TAG = "[CLIENT_INFO]"; -static bool encode_client_mac(pb_ostream_t *stream, const pb_field_t *field, - void *const *arg) { - const uint8_t *mac = (const uint8_t *)*arg; - if (mac == NULL) { - return true; - } - if (!pb_encode_tag_for_field(stream, field)) { - return false; - } - return pb_encode_string(stream, mac, CLIENT_MAC_LEN); -} - static bool encode_clients_list(pb_ostream_t *stream, const pb_field_t *field, - void *const *arg) { + void *const *arg) { (void)arg; size_t count = client_registry_count(); @@ -31,6 +17,8 @@ static bool encode_clients_list(pb_ostream_t *stream, const pb_field_t *field, continue; } + uart_cmd_bytes_t mac = {.data = client->mac, .len = CLIENT_MAC_LEN}; + alox_ClientInfo proto = alox_ClientInfo_init_zero; proto.id = client->id; proto.available = client->available; @@ -39,8 +27,8 @@ static bool encode_clients_list(pb_ostream_t *stream, const pb_field_t *field, proto.last_success_ping = client_registry_ms_since(client->last_success_ping_at); proto.version = client->version; - proto.mac.funcs.encode = encode_client_mac; - proto.mac.arg = (void *)client->mac; + proto.mac.funcs.encode = uart_cmd_encode_bytes; + proto.mac.arg = &mac; if (!pb_encode_tag_for_field(stream, field)) { return false; @@ -56,23 +44,16 @@ static void handle_client_info(const uint8_t *data, size_t len) { (void)data; (void)len; - alox_UartMessage response = alox_UartMessage_init_zero; - response.type = alox_MessageType_CLIENT_INFO; - response.which_payload = alox_UartMessage_client_info_response_tag; - response.payload.client_info_response.clients.funcs.encode = - encode_clients_list; + alox_UartMessage response; + uart_cmd_init_response(&response, alox_MessageType_CLIENT_INFO, + alox_UartMessage_client_info_response_tag); + response.payload.client_info_response.clients.funcs.encode = encode_clients_list; response.payload.client_info_response.clients.arg = NULL; ESP_LOGI(TAG, "sending %u clients", (unsigned)client_registry_count()); - - if (uart_send_uart_message(&response) != ESP_OK) { - ESP_LOGE(TAG, "failed to send response"); - } + uart_cmd_send(&response, TAG); } void cmd_client_info_register(void) { - if (msg_register_handler(alox_MessageType_CLIENT_INFO, handle_client_info) != - ESP_OK) { - ESP_LOGE(TAG, "register failed"); - } + uart_cmd_register(alox_MessageType_CLIENT_INFO, handle_client_info); } diff --git a/main/cmd_espnow_unicast_test.c b/main/cmd_espnow_unicast_test.c index 604e847..f6f3caf 100644 --- a/main/cmd_espnow_unicast_test.c +++ b/main/cmd_espnow_unicast_test.c @@ -1,66 +1,51 @@ #include "client_registry.h" #include "cmd_espnow_unicast_test.h" -#include "cmd_handler.h" #include "esp_log.h" #include "esp_now_comm.h" -#include "pb_decode.h" -#include "uart_messages.pb.h" -#include "uart_proto.h" +#include "uart_cmd.h" static const char *TAG = "[UNICAST_TEST]"; -static void send_response(bool success, uint32_t seq) { - alox_UartMessage response = alox_UartMessage_init_zero; - response.type = alox_MessageType_ESPNOW_UNICAST_TEST; - response.which_payload = alox_UartMessage_espnow_unicast_test_response_tag; +static void reply(bool success, uint32_t seq) { + alox_UartMessage response; + uart_cmd_init_response(&response, alox_MessageType_ESPNOW_UNICAST_TEST, + alox_UartMessage_espnow_unicast_test_response_tag); response.payload.espnow_unicast_test_response.success = success; response.payload.espnow_unicast_test_response.seq = seq; - - if (uart_send_uart_message(&response) != ESP_OK) { - ESP_LOGE(TAG, "failed to send response"); - } + uart_cmd_send(&response, TAG); } static void handle_espnow_unicast_test(const uint8_t *data, size_t len) { - alox_UartMessage uart_msg = alox_UartMessage_init_zero; - alox_EspNowUnicastTestRequest req = alox_EspNowUnicastTestRequest_init_zero; - bool have_request = false; + alox_UartMessage uart_msg; - if (len > 0) { - pb_istream_t stream = pb_istream_from_buffer(data, len); - if (!pb_decode(&stream, alox_UartMessage_fields, &uart_msg)) { - ESP_LOGW(TAG, "decode failed"); - send_response(false, 0); - return; - } - if (uart_msg.which_payload == - alox_UartMessage_espnow_unicast_test_request_tag) { - req = uart_msg.payload.espnow_unicast_test_request; - have_request = true; - } - } - - if (!have_request || req.client_id == 0) { - ESP_LOGW(TAG, "need client_id in request"); - send_response(false, 0); + if (uart_cmd_decode(data, len, &uart_msg) != ESP_OK) { + ESP_LOGW(TAG, "decode failed"); + reply(false, 0); return; } - const client_info_t *client = client_registry_find_by_id(req.client_id); + const alox_EspNowUnicastTestRequest *req = UART_CMD_REQ( + &uart_msg, alox_UartMessage_espnow_unicast_test_request_tag, + espnow_unicast_test_request); + if (req == NULL || req->client_id == 0) { + ESP_LOGW(TAG, "need client_id in request"); + reply(false, 0); + return; + } + + const client_info_t *client = client_registry_find_by_id(req->client_id); if (client == NULL) { ESP_LOGW(TAG, "client id %lu not in registry", - (unsigned long)req.client_id); - send_response(false, req.seq); + (unsigned long)req->client_id); + reply(false, req->seq); return; } - esp_err_t err = esp_now_comm_send_unicast_test(client->mac, req.seq); - send_response(err == ESP_OK, req.seq); + esp_err_t err = esp_now_comm_send_unicast_test(client->mac, req->seq); + reply(err == ESP_OK, req->seq); } void cmd_espnow_unicast_test_register(void) { - if (msg_register_handler(alox_MessageType_ESPNOW_UNICAST_TEST, - handle_espnow_unicast_test) != ESP_OK) { - ESP_LOGE(TAG, "register failed"); - } + uart_cmd_register(alox_MessageType_ESPNOW_UNICAST_TEST, + handle_espnow_unicast_test); } diff --git a/main/cmd_version.c b/main/cmd_version.c index 6d6246c..ebd2687 100644 --- a/main/cmd_version.c +++ b/main/cmd_version.c @@ -1,10 +1,5 @@ -#include "cmd_handler.h" #include "cmd_version.h" -#include "esp_log.h" -#include "pb_encode.h" -#include "uart_messages.pb.h" -#include "uart_proto.h" -#include +#include "uart_cmd.h" #ifndef POWERPOD_FW_VERSION #define POWERPOD_FW_VERSION 1u @@ -16,36 +11,19 @@ static const char *TAG = "[VERSION]"; -static bool encode_git_hash(pb_ostream_t *stream, const pb_field_t *field, - void *const *arg) { - const char *str = (const char *)*arg; - if (str == NULL) { - str = ""; - } - if (!pb_encode_tag_for_field(stream, field)) { - return false; - } - return pb_encode_string(stream, (const pb_byte_t *)str, strlen(str)); -} - static void handle_version(const uint8_t *data, size_t len) { (void)data; (void)len; - alox_UartMessage response = alox_UartMessage_init_zero; - response.type = alox_MessageType_VERSION; - response.which_payload = alox_UartMessage_version_response_tag; + alox_UartMessage response; + uart_cmd_init_response(&response, alox_MessageType_VERSION, + alox_UartMessage_version_response_tag); response.payload.version_response.version = POWERPOD_FW_VERSION; - response.payload.version_response.git_hash.funcs.encode = encode_git_hash; + response.payload.version_response.git_hash.funcs.encode = uart_cmd_encode_string; response.payload.version_response.git_hash.arg = (void *)POWERPOD_GIT_HASH; - - if (uart_send_uart_message(&response) != ESP_OK) { - ESP_LOGE(TAG, "failed to send response"); - } + uart_cmd_send(&response, TAG); } void cmd_version_register(void) { - if (msg_register_handler(alox_MessageType_VERSION, handle_version) != ESP_OK) { - ESP_LOGE(TAG, "register failed"); - } + uart_cmd_register(alox_MessageType_VERSION, handle_version); } diff --git a/main/uart_cmd.c b/main/uart_cmd.c new file mode 100644 index 0000000..a3caee5 --- /dev/null +++ b/main/uart_cmd.c @@ -0,0 +1,82 @@ +#include "uart_cmd.h" +#include "esp_log.h" +#include "pb_decode.h" +#include "pb_encode.h" +#include "uart_proto.h" +#include + +static const char *TAG = "[UART_CMD]"; + +esp_err_t uart_cmd_decode(const uint8_t *data, size_t len, alox_UartMessage *out) { + if (out == NULL) { + return ESP_ERR_INVALID_ARG; + } + + alox_UartMessage zero = alox_UartMessage_init_zero; + *out = zero; + if (len == 0) { + return ESP_OK; + } + if (data == NULL) { + return ESP_ERR_INVALID_ARG; + } + + pb_istream_t stream = pb_istream_from_buffer(data, len); + if (!pb_decode(&stream, alox_UartMessage_fields, out)) { + return ESP_FAIL; + } + return ESP_OK; +} + +void uart_cmd_init_response(alox_UartMessage *msg, alox_MessageType type, + pb_size_t which_payload) { + if (msg == NULL) { + return; + } + alox_UartMessage zero = alox_UartMessage_init_zero; + *msg = zero; + msg->type = type; + msg->which_payload = which_payload; +} + +esp_err_t uart_cmd_send(const alox_UartMessage *msg, const char *log_tag) { + esp_err_t err = uart_send_uart_message(msg); + if (err != ESP_OK && log_tag != NULL) { + ESP_LOGE(log_tag, "failed to send UART response"); + } + return err; +} + +esp_err_t uart_cmd_register(alox_MessageType type, msg_callback_t cb) { + esp_err_t err = msg_register_handler((uint16_t)type, cb); + if (err != ESP_OK) { + ESP_LOGE(TAG, "register handler for type %u failed", (unsigned)type); + } + return err; +} + +bool uart_cmd_encode_string(pb_ostream_t *stream, const pb_field_t *field, + void *const *arg) { + const char *str = (const char *)*arg; + if (str == NULL) { + str = ""; + } + if (!pb_encode_tag_for_field(stream, field)) { + return false; + } + return pb_encode_string(stream, (const pb_byte_t *)str, strlen(str)); +} + +bool uart_cmd_encode_bytes(pb_ostream_t *stream, const pb_field_t *field, + void *const *arg) { + const uart_cmd_bytes_t *blob = (const uart_cmd_bytes_t *)*arg; + + (void)field; + if (blob == NULL || blob->data == NULL) { + return true; + } + if (!pb_encode_tag_for_field(stream, field)) { + return false; + } + return pb_encode_string(stream, blob->data, blob->len); +} diff --git a/main/uart_cmd.h b/main/uart_cmd.h new file mode 100644 index 0000000..db1fc4f --- /dev/null +++ b/main/uart_cmd.h @@ -0,0 +1,37 @@ +#ifndef UART_CMD_H +#define UART_CMD_H + +#include "cmd_handler.h" +#include "esp_err.h" +#include "pb.h" +#include "uart_messages.pb.h" + +/** Decode framed UART body (protobuf UartMessage without the leading type byte). */ +esp_err_t uart_cmd_decode(const uint8_t *data, size_t len, alox_UartMessage *out); + +/** Set type + oneof tag on an outbound UartMessage. */ +void uart_cmd_init_response(alox_UartMessage *msg, alox_MessageType type, + pb_size_t which_payload); + +/** Encode and send; logs on failure when log_tag is non-NULL. */ +esp_err_t uart_cmd_send(const alox_UartMessage *msg, const char *log_tag); + +esp_err_t uart_cmd_register(alox_MessageType type, msg_callback_t cb); + +/** Nanopb encode callback for a NUL-terminated C string. */ +bool uart_cmd_encode_string(pb_ostream_t *stream, const pb_field_t *field, + void *const *arg); + +typedef struct { + const uint8_t *data; + size_t len; +} uart_cmd_bytes_t; + +/** Nanopb encode callback; arg is uart_cmd_bytes_t *. */ +bool uart_cmd_encode_bytes(pb_ostream_t *stream, const pb_field_t *field, + void *const *arg); + +#define UART_CMD_REQ(msg, tag, field) \ + ((msg)->which_payload == (tag) ? &(msg)->payload.field : NULL) + +#endif