[serial_proxy] Enforce session scoping and RAW inertness for the port mode

Address review findings:
- Only the live subscriber may set the mode, so a mode set by a client
  that never subscribes cannot persist past its session
- With a subscriber attached, the mode alone decides whether the tap is
  served; tap_needs_port() bypasses it only while the port is unheld,
  and write_from_tap() is gated the same way, so RAW is inert by code
- The explicit UNSUBSCRIBE path keeps the loop alive for a tap that
  still needs the port, mirroring the disconnect path in loop()
- Mode values from the wire are validated; unknown values are refused
  with INVALID_ARGUMENT instead of stored and acknowledged OK
- Add a test variant that defines USE_SERIAL_PROXY_TAP so the tap code
  paths compile in a real build
This commit is contained in:
kbx81
2026-09-02 22:27:52 -05:00
parent 6ae5f070c8
commit 88402743d5
4 changed files with 44 additions and 11 deletions
+1
View File
@@ -2851,6 +2851,7 @@ enum SerialProxyMode {
SERIAL_PROXY_MODE_PROTOCOL = 1;
}
// Only the subscribed client may change the mode; others are refused with PORT_IN_USE.
message SerialProxySetModeRequest {
option (id) = 152;
option (source) = SOURCE_CLIENT;
@@ -120,11 +120,10 @@ bool SerialProxy::tap_observing_() const {
if (this->tap_ == nullptr) {
return false;
}
// A tap that needs the port is mid-protocol-work of its own -- the boot-time handshake
// with the device, which runs before any client has connected and so before anyone could
// have chosen a mode. Withholding bytes from it there would strand it, so it is served
// regardless of mode.
if (this->tap_->tap_needs_port()) {
// With no subscriber, a tap doing its own protocol work (the boot-time handshake with
// the device, say) is served regardless of mode -- nobody has chosen one yet. Once a
// subscriber holds the port, the mode alone decides, so RAW stays inert.
if (this->api_connection_ == nullptr && this->tap_->tap_needs_port()) {
return true;
}
// Otherwise the mode decides. RAW must be inert: a client that flips to RAW before
@@ -233,13 +232,19 @@ SerialProxyResult SerialProxy::configure(api::APIConnection *api_connection, uin
SerialProxyResult SerialProxy::set_mode(api::APIConnection *api_connection, api::enums::SerialProxyMode mode) {
#ifdef USE_API
if (this->port_claimed_by_other_(api_connection)) {
ESP_LOGW(TAG, "Ignoring mode request from client without port access [%" PRIu32 "]", this->instance_index_);
// Only the live subscriber may change the mode, so the mode cannot outlive a session
if (this->api_connection_ != api_connection) {
ESP_LOGW(TAG, "Ignoring mode request from client without port subscription [%" PRIu32 "]", this->instance_index_);
return SerialProxyResult::SERIAL_PROXY_RESULT_PORT_IN_USE;
}
#endif
// Values come from a remote client
if (mode != api::enums::SERIAL_PROXY_MODE_RAW && mode != api::enums::SERIAL_PROXY_MODE_PROTOCOL) {
ESP_LOGW(TAG, "Invalid mode: %" PRIu32, static_cast<uint32_t>(mode));
return SerialProxyResult::SERIAL_PROXY_RESULT_INVALID_ARGUMENT;
}
ESP_LOGD(TAG, "Serial proxy [%" PRIu32 "] mode set to %s", this->instance_index_,
mode == api::enums::SERIAL_PROXY_MODE_PROTOCOL ? "PROTOCOL" : "RAW");
mode == api::enums::SERIAL_PROXY_MODE_PROTOCOL ? LOG_STR_LITERAL("PROTOCOL") : LOG_STR_LITERAL("RAW"));
const bool leaving_protocol_mode =
this->mode_ != api::enums::SERIAL_PROXY_MODE_RAW && mode == api::enums::SERIAL_PROXY_MODE_RAW;
this->mode_ = mode;
@@ -368,7 +373,14 @@ SerialProxyResult SerialProxy::serial_proxy_request(api::APIConnection *api_conn
}
this->api_connection_ = nullptr;
this->reset_mode_();
#ifdef USE_SERIAL_PROXY_TAP
// Keep the loop alive for a tap that still needs the port (mirrors loop())
if (this->tap_ == nullptr || !this->tap_->tap_needs_port()) {
this->disable_loop();
}
#else
this->disable_loop();
#endif
ESP_LOGV(TAG, "API connection unsubscribed from serial proxy [%" PRIu32 "]", this->instance_index_);
return SerialProxyResult::SERIAL_PROXY_RESULT_OK;
default:
@@ -71,7 +71,8 @@ class SerialProxyTap {
virtual void on_client_tx(const uint8_t *data, size_t len) = 0;
/// True when the port must keep reading even with no subscriber attached, so a tap can
/// do its own protocol work while nobody is listening.
/// do its own protocol work while nobody is listening. Honoured only while no
/// subscriber holds the port; with one attached, the port mode alone decides.
virtual bool tap_needs_port() const = 0;
/// A client explicitly turned protocol handling off for this port. Distinct from the
@@ -165,8 +166,13 @@ class SerialProxy final : public uart::UARTDevice, public Component {
void set_tap(SerialProxyTap *tap) { this->tap_ = tap; }
/// Write bytes originating from the tap rather than from a client. Bypasses the
/// subscriber ownership check, since the tap is part of the device, not a client of it.
void write_from_tap(const uint8_t *data, size_t len) { this->write_array(data, len); }
/// subscriber ownership check, but only while the tap is being served bytes -- so a
/// port in RAW mode with a subscriber attached stays inert.
void write_from_tap(const uint8_t *data, size_t len) {
if (this->tap_observing_()) {
this->write_array(data, len);
}
}
/// Resume reading after a tap's needs change. loop() disables itself when there is
/// neither a subscriber nor a tap that wants the port, so a tap starting fresh work
@@ -0,0 +1,14 @@
substitutions:
tx_pin: GPIO4
rx_pin: GPIO5
# Compile the tap code paths; no tap is attached, so this exercises the
# null-tap branches that a normal build never defines.
esphome:
platformio_options:
build_flags:
- "-DUSE_SERIAL_PROXY_TAP"
packages:
uart: !include ../../test_build_components/common/uart/esp32-idf.yaml
serial_proxy: !include common.yaml