Feature/serial bridge subscribe #332

matt001k · 2025-08-18T17:00:25Z

What changed?

Added subscription capability to serial bridge application.
Adds serial bridge application to Bristleback.

How does it make Bristlemouth better?

Expands the serial bridge application to allow companion devices to subscribe to topics.

Where should reviewers focus?

Mainly the diff in the serial bridge apps!

Checklist

Add or update unit tests for changed code
Ensure all submodules up to date. If this PR relies on changes in submodules, merge those PRs first, then point this PR at/after the merge commit
Ensure code is formatted correctly with clang-format. If there are large formatting changes, they should happen in a separate whitespace-only commit on this PR after all approvals.

LarkspurRaven · 2025-08-26T00:20:45Z

src/apps/bristleback_apps/serial_bridge/user_code/user_code.h

@@ -0,0 +1,4 @@
+#pragma once
+
+void setup();


Add comments for functions. Though, this interface is used in many other places. Thinking this common interface could go somewhere like src/lib/user_app

I think this is supposed to mimic an Arduino like interface for folks looking to quickly dev on the Bristlemouth development kits. Hence why all of the user_code files follow this trend. I would take a look at some of the Bristlemouth DevKit instructions for users looking to develop in the Bristlemouth ecosystem if you haven't taken a look yet!

LarkspurRaven · 2025-08-26T00:21:53Z

src/apps/bristleback_apps/serial_bridge/user_code/user_code.cpp

+#include "sensors.h"
+#include "task_priorities.h"
+#include <inttypes.h>
+#include <string.h>


This doesn't include user_code.h.

<inttypes.h> appears to not be used. I think standard int types are coming from <stdin.h> via bm_os.h

LarkspurRaven · 2025-08-26T00:44:59Z

src/apps/bristleback_apps/serial_bridge/user_code/user_code.cpp

+typedef enum {
+  BM_NONE,
+  BM_SUB,
+} __attribute__((__packed__)) bm_serial_tx_message_t;


This could change to

__PACKED_STRUCT bm_serial_tx_message_t { // fields here };

wait -- bm_serial_tx_message_t is an enum. I don't think it makes sense to make an enum a packed struct. Did you mean to put that on bm_serial_tx_t?

This is unused and has been removed.

LarkspurRaven · 2025-08-26T00:49:46Z

src/apps/bristleback_apps/serial_bridge/user_code/user_code.cpp

+}
+
+void setup() {
+  PLUART::init(USER_TASK_PRIORITY);


What happens if PLUART is used by another task?

Looking at payload_uart::init(), I have some concern there.

I suppose PLUART is attempting to do a Singleton pattern, but there is nothing I see that prevents it from being called by multiple users, each potentially changing the setup

The init function just allocates a new task, but there is no way I see to destroy the task.

For all cases, payload_uart::init() configures MX_LPUART1_UART_Init, but then we are allowed to set the other parameters such as baud, useByteStreamBuffer, etc and also independantly enable/disable

There is nothing that calls payload_uart::disable(). But even if we did, it still does not clean up the FreeRTOS task. Perhaps it shouldnt' for disable, but the name should be called disable_serial(). Perhaps there could be a seperate function disable_serial_task() -- though, that wouldn't just work as pluart is designed because no handle to the task is returned to the caller.

Outside the scope of this PR, but I don't understand why payload_uart was hardcoded for a specific uart

actually, it's worse user_byte_stream_buffer is static and is reset on each call of init() (same for _user_line, _postTxSemaphore, uart_handle). It may still work ok, but this would be a leak for multiple users. Granted, out of scope for this PR, but unless I'm crazy, if pluart is intended to be a singleton with one client, init should be something like this

// Check if already initialized if (rxTaskHandle != NULL) { return pdFAIL; // Already initialized }

though it may be okay to allow mutliple callers with one instance. In the current design, allowing multiple tasks doesn't make sense because we are hard-binded to one specific uart.

man...pluart also exposes these in the header

// external declarations for the variables extern SerialLineBuffer_t lpUART1LineBuffer; extern SerialHandle_t uart_handle;

these need moved to the cpp file

@matt001k sorry for these pluart comments! I just don't want to lose the thoughts!

hmm...seeing the vTaskDelay(pdMS_TO_TICKS(5)); code in the setup function made me think that we may actually have the context for the task here, so we may be able to teardown the task from here. Though the memory leaks in pluart remain

Definitely worth capturing, right now there is no contention in any of our apps but could for sure be in the future.

LarkspurRaven · 2025-08-26T01:04:12Z

src/apps/bristleback_apps/serial_bridge/user_code/user_code.cpp

+#include <inttypes.h>
+#include <string.h>
+
+typedef enum {


Add comments to bm_serial_tx_message_t, bm_serial_tx_message_t, and the variables declarations below. Add new lines after the enum & struct declarations.

This typedef is unused and has been removed.

LarkspurRaven · 2025-08-26T01:04:39Z

src/apps/bristleback_apps/serial_bridge/user_code/user_code.cpp

+static constexpr size_t packet_buf_size = rx_buf_size;
+static uint8_t packet_buffer[rx_buf_size];
+
+static bool tx_cb(const uint8_t *buff, size_t len, bm_serial_message_t message) {


Add comments for each internal method

LarkspurRaven · 2025-08-26T01:05:31Z

src/apps/bristleback_apps/serial_bridge/user_code/user_code.cpp

+  IOWrite(&BB_PL_BUCK_EN, 0);
+}
+
+static void reset() {


reset_rx may be more clear b/c that is all it is resetting

should we reset packet_buffer here too?

LarkspurRaven · 2025-08-26T01:18:25Z

src/apps/bristleback_apps/serial_bridge/user_code/user_code.cpp

+  uint8_t type;
+  uint32_t length;
+  uint8_t data[0];
+} bm_serial_tx_t;


bm_serial_tx_t doesn't seem to be used. If correct, can remove it.

LarkspurRaven · 2025-08-26T01:27:11Z

src/apps/bristleback_apps/serial_bridge/user_code/user_code.cpp

+  callbacks.pub_fn = pub_cb;
+  callbacks.sub_fn = sub_cb;
+  callbacks.tx_fn = tx_cb;


The callbacks could be defined compile time

static bm_serial_callbacks_t callbacks = { callbacks.pub_fn = pub_cb, callbacks.sub_fn = sub_cb, callbacks.tx_fn = tx_cb;, }

you'd just have to forward declare the functions

I do have a slight concern for namespace collection with the generic function names pub_sub, sub_cb, tx_cb. We should be safe for compile b/c they are delcared static. Something like serial_bridge_pub_cb could make it easier to read and trace.

LarkspurRaven · 2025-08-26T01:28:20Z

src/apps/bristleback_apps/serial_bridge/user_code/user_code.cpp

@@ -0,0 +1,100 @@
+#include "bm_os.h"


I think it'd be more descriptive to name this file something like serial_bridge rather than the generic user_code.cpp. Though I realize this is a common pattern.

towynlin · 2025-08-28T17:18:00Z

Good comments, thanks @LarkspurRaven — this PR is low priority and does not need to make it into v0.13.4.

LarkspurRaven

not quite done with review, but progress so far

LarkspurRaven · 2025-08-26T02:14:13Z

src/apps/bristleback_apps/serial_bridge/user_code/user_code.cpp

+  uint32_t length;
+  uint8_t data[0];
+} bm_serial_tx_t;
+static bm_serial_callbacks_t callbacks;


Add comments for variable declarations

LarkspurRaven · 2025-08-26T02:16:00Z

src/apps/bristleback_apps/serial_bridge/user_code/user_code.cpp

+  uint8_t data[0];
+} bm_serial_tx_t;
+static bm_serial_callbacks_t callbacks;
+static constexpr size_t rx_buf_size = 2048;


rx_buf_size s/b capitalized --> RX_BUF_SIZE. Well, not 100% certain on our naming standards...need to check

As we are using this for both rx_buf and pkt_buff, could be better to name as BUFFER_SIZE

This is just my preference, but I prefer to have configurable values like this declared at the top of the file in a marked section. Perhaps in a header. Is this buffer size common for other user_code apps?

LarkspurRaven · 2025-08-26T02:18:42Z

src/apps/bristleback_apps/serial_bridge/user_code/user_code.cpp

+static uint8_t packet_buffer[rx_buf_size];
+
+static bool tx_cb(const uint8_t *buff, size_t len, bm_serial_message_t message) {
+  (void)message;


should probably add a check on buff for null pointer, since it is not coming from us. Perhaps add a check for length as well. Though maybe PLUART::write() checks both.

I don't really understand who calls this method tx_cb(), or when it'd be called. Comment could clarify that.

LarkspurRaven · 2025-08-26T02:21:24Z

src/apps/bristleback_apps/serial_bridge/user_code/user_code.cpp

+  return true;
+}
+
+static void sub_message_cb(uint64_t node_id, const char *topic, uint16_t topic_len,


in sub_message_cb() are topic and data guaranteed to not be null? If not, we should probably add a null check. is there any validation we could consider on the topic and data?

Perhaps bm_serial_pub() handles my concerns? If so, we should call it first

bm_serial_pub does check the params, so we should call it first

if(bm_serial_pub(...){ printf("Failed to send serial_bridge msg\n"); some_method_to_safely_print_this_message(...); } else { printf("Publishing To Topic: %s\n", topic); }

LarkspurRaven · 2025-08-26T02:30:03Z

src/apps/bristleback_apps/serial_bridge/user_code/user_code.cpp

+}
+
+static bool sub_cb(const char *topic, uint16_t topic_len) {
+  return bm_sub_wl(topic, topic_len, sub_message_cb);


bm_sub_wl returns BmErr not bool. Should cast or handle

LarkspurRaven · 2025-08-26T02:30:27Z

src/apps/bristleback_apps/serial_bridge/user_code/user_code.cpp

+                   const uint8_t *payload, size_t len, uint8_t type, uint8_t version) {
+  (void)node_id;
+  printf("publishing %u bytes to %.*s\n", len, topic_len, topic);
+  return bm_pub_wl(topic, topic_len, payload, len, type, version) == BmOK;


bm_pub_wl returns BmErr not bool. Should cast or handle

Correct, the statements reads:

bm_pub_wl(topic, topic_len, payload, len, type, version) == BmOK

my mistake 👍

LarkspurRaven · 2025-08-26T02:33:10Z

src/apps/bristleback_apps/serial_bridge/user_code/user_code.cpp

+}
+
+void setup() {
+  PLUART::init(USER_TASK_PRIORITY);


hmm...seeing the vTaskDelay(pdMS_TO_TICKS(5)); code in the setup function made me think that we may actually have the context for the task here, so we may be able to teardown the task from here. Though the memory leaks in pluart remain

matt001k · 2025-09-03T00:09:14Z

src/apps/bristleback_apps/serial_bridge/user_code/user_code.cpp

+#include <inttypes.h>
+#include <string.h>
+
+typedef enum {


This typedef is unused and has been removed.

matt001k · 2025-09-03T00:09:35Z

src/apps/bristleback_apps/serial_bridge/user_code/user_code.cpp

+typedef enum {
+  BM_NONE,
+  BM_SUB,
+} __attribute__((__packed__)) bm_serial_tx_message_t;


This is unused and has been removed.

matt001k · 2025-09-03T00:14:56Z

src/apps/bristleback_apps/serial_bridge/user_code/user_code.cpp

@LarkspurRaven this file was copied from another application. I will make suggested changes but it will not be reflected in the other user_code.cpp file located in this diff.
There should probably be some common location for application that can exist in multiple application spaces.

matt001k · 2025-09-03T00:42:27Z

src/apps/bristleback_apps/serial_bridge/user_code/user_code.cpp

+                   const uint8_t *payload, size_t len, uint8_t type, uint8_t version) {
+  (void)node_id;
+  printf("publishing %u bytes to %.*s\n", len, topic_len, topic);
+  return bm_pub_wl(topic, topic_len, payload, len, type, version) == BmOK;


Correct, the statements reads:

bm_pub_wl(topic, topic_len, payload, len, type, version) == BmOK

matt001k · 2025-09-03T00:43:22Z

src/apps/bristleback_apps/serial_bridge/user_code/user_code.cpp

+}
+
+void setup() {
+  PLUART::init(USER_TASK_PRIORITY);


Definitely worth capturing, right now there is no contention in any of our apps but could for sure be in the future.

NA

matt001k requested review from LarkspurRaven, towynlin and victorsowa12 August 25, 2025 16:59

matt001k added the enhancement New feature or request label Aug 25, 2025

victorsowa12 approved these changes Aug 25, 2025

View reviewed changes

towynlin approved these changes Aug 26, 2025

View reviewed changes

LarkspurRaven previously requested changes Aug 26, 2025

View reviewed changes

LarkspurRaven reviewed Aug 28, 2025

View reviewed changes

matt001k commented Sep 3, 2025

View reviewed changes

matt001k added 6 commits November 3, 2025 16:52

Adds Subscription Ability To Serial Bridge Application

a0c97b2

Add Bristleback Serial Bridge App

fb3c3db

Update application based on PR feedback

eae3296

Address more PR comments

e4a500b

Create common serial bridge location

91aa52c

Get it building ok ok?

6fdfdb4

matt001k force-pushed the feature/serial_bridge_subscribe branch from d89bf21 to 6fdfdb4 Compare November 4, 2025 00:53

Update BM Serial

3256e99

Feature/serial bridge subscribe #332

Are you sure you want to change the base?

Feature/serial bridge subscribe #332

Uh oh!

Conversation

matt001k commented Aug 18, 2025

What changed?

How does it make Bristlemouth better?

Where should reviewers focus?

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LarkspurRaven Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

towynlin commented Aug 28, 2025

Uh oh!

LarkspurRaven left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LarkspurRaven Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LarkspurRaven Aug 26, 2025 •

edited

Loading

LarkspurRaven Aug 26, 2025 •

edited

Loading