[entity_transport] Fixes from code review

- Check a data packet before asking for its dictionary, so a damaged
  packet does not make the provider resend everything
- When a provider's complete dictionary does not name a remote entity,
  warn and clear that entity's state, as a typo in provider or remote_id,
  or an entity the provider no longer sends, was otherwise silent

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
clydebarrow
2026-10-20 08:41:48 +11:00
co-authored by Claude Opus 5.5
parent 88554b0354
commit b4be2d8aa3
3 changed files with 75 additions and 7 deletions
@@ -436,7 +436,8 @@ void EntityTransport::receive_dictionary_(std::span<const uint8_t> packet, uint3
bool in_order = index == 0 || (this->rx_dictionary_id_ == dictionary_id && index == this->rx_next_part_);
this->rx_dictionary_id_ = dictionary_id;
this->rx_next_part_ = in_order ? (index + 1) & PART_INDEX_MASK : PART_NONE;
if (in_order && (part & PART_LAST) != 0 && !this->dictionary_seen_(dictionary_id)) {
bool completed = in_order && (part & PART_LAST) != 0 && !this->dictionary_seen_(dictionary_id);
if (completed) {
this->seen_dictionaries_[this->seen_next_] = dictionary_id;
this->seen_next_ = (this->seen_next_ + 1) % this->seen_dictionaries_.size();
}
@@ -455,6 +456,37 @@ void EntityTransport::receive_dictionary_(std::span<const uint8_t> packet, uint3
pos += id_length;
}
}
if (completed)
this->forget_unnamed_(provider, provider_length, dictionary_id);
}
void EntityTransport::forget_unnamed_(const char *provider, size_t provider_length, uint32_t dictionary_id) {
// A remote entity that its provider's whole dictionary does not name would otherwise keep no or a stale state
// silently
#ifdef ENTITY_TRANSPORT_REMOTE_SENSOR_COUNT
for (auto &remote : this->remote_sensors_) {
if (!matches(remote.provider, provider, provider_length) || (remote.known && remote.dictionary_id == dictionary_id))
continue;
ESP_LOGW(TAG, "'%s' does not send '%s'", remote.provider, remote.remote_id);
remote.known = false;
if (remote.has_state) {
remote.has_state = false;
remote.entity->publish_state(NAN);
}
}
#endif
#ifdef ENTITY_TRANSPORT_REMOTE_BINARY_SENSOR_COUNT
for (auto &remote : this->remote_binary_sensors_) {
if (!matches(remote.provider, provider, provider_length) || (remote.known && remote.dictionary_id == dictionary_id))
continue;
ESP_LOGW(TAG, "'%s' does not send '%s'", remote.provider, remote.remote_id);
remote.known = false;
if (remote.has_state) {
remote.has_state = false;
remote.entity->invalidate_state();
}
}
#endif
}
void EntityTransport::learn_(const char *provider, size_t provider_length, uint8_t type, uint8_t number, const char *id,
@@ -503,18 +535,21 @@ void EntityTransport::receive_data_(std::span<const uint8_t> packet, uint32_t di
has_remotes = true;
}
#endif
// A dictionary not yet received in full may name our entities, or be a provider's new one; one received in full
// named all of them it could, so asking again would only make that provider resend everything
if (has_remotes && !this->dictionary_seen_(dictionary_id))
this->request_dictionary_(dictionary_id);
if (!known)
if (!has_remotes)
return;
// Checked first, so a damaged packet does not make the provider resend everything
if (!data_well_formed(packet)) {
if (this->warning_due_()) {
ESP_LOGW(TAG, "Data packet of %zu bytes is malformed", packet.size());
}
return;
}
// A dictionary not yet received in full may name our entities, or be a provider's new one; one received in full
// named all of them it could, so asking again would only make that provider resend everything
if (!this->dictionary_seen_(dictionary_id))
this->request_dictionary_(dictionary_id);
if (!known)
return;
[[maybe_unused]] uint32_t now = App.get_loop_component_start_time();
for (size_t pos = HEADER_SIZE; pos != packet.size(); pos += record_size(packet, pos)) {
[[maybe_unused]] Record record = parse_record(packet, pos);
@@ -125,6 +125,8 @@ class EntityTransport : public Component {
size_t id_length, uint32_t dictionary_id);
void request_dictionary_(uint32_t dictionary_id);
bool dictionary_seen_(uint32_t dictionary_id) const;
/// Warns about, and clears, each remote entity of \p provider that its complete dictionary does not name.
void forget_unnamed_(const char *provider, size_t provider_length, uint32_t dictionary_id);
bool warning_due_();
void on_session_(bool connected);
void clear_remote_states_(bool timed_out_only);
@@ -226,6 +226,37 @@ TEST_F(EntityTransportTest, ChangedDictionaryRequested) {
EXPECT_EQ(this->remote_temp_.get_state(), 6.0f);
}
TEST_F(EntityTransportTest, MalformedDataNotRequested) {
auto receiver = this->make_receiver_();
// A data packet under an unknown dictionary, cut off inside its first record
Packet data = header(PACKET_DATA, 0x12345678);
data.insert(data.end(), {0x00, 0x21, 0x00});
this->link_b_.inject(data);
EXPECT_TRUE(this->link_b_.sent.empty());
}
TEST_F(EntityTransportTest, EntityLeftOutOfNewDictionaryCleared) {
this->temp_.publish_state(5.0f);
this->door_.publish_state(true);
auto sender = this->make_sender_();
auto receiver = this->make_receiver_();
sender->loop();
this->link_a_.deliver_to(this->link_b_);
EXPECT_TRUE(this->remote_door_.state);
// The provider restarts without the door
sensor::Sensor temp;
MockInterface link;
TestTransport changed(&link);
changed.set_entity_count(1);
changed.add_sensor("temp", &temp);
changed.setup();
temp.publish_state(6.0f);
changed.loop();
link.deliver_to(this->link_b_);
EXPECT_EQ(this->remote_temp_.get_state(), 6.0f);
EXPECT_FALSE(this->remote_door_.has_state());
}
TEST_F(EntityTransportTest, LostDictionaryPartRequested) {
// One entity in each dictionary packet
MockInterface link(5 + 2 + strlen(DEVICE_NAME) + 3 + 5);
@@ -271,7 +302,7 @@ TEST_F(EntityTransportTest, RefusedRequestSentAgain) {
TEST_F(EntityTransportTest, RequestsLimited) {
auto receiver = this->make_receiver_();
Packet data = header(PACKET_DATA, 0x12345678);
data.push_back(0);
data.insert(data.end(), {0x00, 0x61}); // sensor 0 with no state
this->link_b_.inject(data);
this->link_b_.inject(data);
EXPECT_EQ(this->link_b_.sent.size(), 1);