Skip to content

Commit 000afc5

Browse files
committed
fix(77): address the Copilot review, ship ameba v1.3.3
Two real bugs and one misleading comment, all found by the automated review on PR #68. Both bugs are ameba-only: the esp32 half already had the correct shape in each case. ## Success path diverged from the unified teardown (ameba) RecommissionFabricDelegate::OnFabricCommitted did its own partial cleanup -- clear the flag, cancel the timer, delete the old fabrics -- and never called recommission_finish(). So a SUCCESSFUL re-pair left the CHIP commissioning window OPEN and the BLE advert up, diverging from the expiry / user-cancel / toggle paths. That is precisely the inconsistency the single teardown was introduced to remove, and the comment above it already claimed to cover "every exit". Now routes through recommission_finish(true, ...). The doomed fabric indices are snapshotted first, because recommission_finish() clears the count and Delete() can re-enter the delegate. The esp32 half was already written this way; the port missed it. ## Truncated monotonic timestamp (ameba) The grace deadline stored GetMonotonicTimestamp().count() into a uint32_t and compared truncated counts. That is unit-ambiguous and can wrap, and it ignored the Clock::Timestamp + Milliseconds32 pattern this same file already uses for s_sync_hold_until. Now uses Clock::Timestamp arithmetic throughout. Not an issue on esp32, which uses esp_timer_get_time() -- int64 microseconds, unambiguous. ## Misleading flavour comment (esp32-release.sh) The comment said "pass --release to opt out" but the script parses no such flag; flavour is controlled by the ESP32_FLAVOUR environment variable. Corrected, since an operator following the comment would silently get the debug default -- the opposite of what they asked for. ## Also noted by the review, fixed in the PR description rather than the code The description still described the release as v1.3.0 and claimed swing was EXCLUDED from the "77" exit check. Both are stale: the version is 1.3.3 and swing is deliberately included again (the grace period is what prevents the self-cancel -- excluding swing removed the only exit route that worked). Description updated. Ameba 1.3.2 -> 1.3.3 (int 10303), flashed to node 14 over Matter OTA and verified: avail=True, endpoints {0..10}. Host tests 330 pass. esp32 builds clean but is NOT yet flashed -- USB was disconnected -- and is unaffected by both bugs above. Assisted-by: AI
1 parent 4f6c897 commit 000afc5

3 files changed

Lines changed: 24 additions & 14 deletions

File tree

firmware/scripts/esp32-release.sh

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,8 @@ build() {
125125
# console + `tx` bench probe are gated on CONFIG_HISENSE_DEBUG_BUILD, which lives ONLY in
126126
# sdkconfig.debug. Building without that overlay silently ships an image with no console -- and
127127
# on a node whose Matter link is flaky, that console is the only way in. Default to debug here
128-
# for exactly that reason; pass --release to opt out.
128+
# for exactly that reason. Opt out with ESP32_FLAVOUR=release (an env var, NOT a --release
129+
# flag -- this script does not parse one).
129130
local sdkdef="sdkconfig.defaults"
130131
if [ "${ESP32_FLAVOUR:-debug}" = "debug" ]; then
131132
sdkdef="sdkconfig.defaults;sdkconfig.debug"

firmware/src/sdk-edits/matter_drivers.cpp

Lines changed: 21 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -300,8 +300,8 @@ static bool s_recommission_pending = false;
300300
/* Remote activity is ignored until this deadline after the window opens: the "77" entry gesture
301301
* ("Horizon Airflow x6") is itself a burst of remote presses. Without it the mode cancels itself
302302
* the instant it opens -- measured on the esp32 half, every single attempt. */
303-
static const uint32_t kRecommissionGraceMs = 6000;
304-
static uint32_t s_recommission_grace_ms = 0;
303+
static const chip::System::Clock::Milliseconds32 kRecommissionGrace = chip::System::Clock::Milliseconds32(6000);
304+
static chip::System::Clock::Timestamp s_recommission_grace_until = chip::System::Clock::kZero;
305305
static chip::FabricIndex s_old_fabrics[16];
306306
static uint8_t s_old_fabric_count = 0;
307307

@@ -321,13 +321,22 @@ class RecommissionFabricDelegate : public chip::FabricTable::Delegate
321321
}
322322
ChipLogProgress(DeviceLayer, "recommission: new fabric %u joined -> deleting %u old fabric(s)",
323323
newIndex, s_old_fabric_count);
324-
s_recommission_pending = false;
325-
chip::DeviceLayer::SystemLayer().CancelTimer(recommission_timeout, nullptr);
326-
for (uint8_t i = 0; i < s_old_fabric_count; i++) {
327-
chip::Server::GetInstance().GetFabricTable().Delete(s_old_fabrics[i]);
328-
}
329-
s_old_fabric_count = 0;
324+
/* Route the SUCCESS path through the same teardown as every other exit. It previously did
325+
* its own partial cleanup -- flags + timer only -- which left the CHIP commissioning
326+
* window OPEN and the BLE advert up after a successful re-pair, diverging from the
327+
* expiry/user-cancel paths. That is exactly the inconsistency the unified teardown exists
328+
* to remove. (Copilot review, PR #68.)
329+
*
330+
* Snapshot the doomed indices first: recommission_finish() clears the count, and
331+
* Delete() can re-enter this delegate. */
332+
chip::FabricIndex doomed[16];
333+
uint8_t n = s_old_fabric_count;
334+
for (uint8_t i = 0; i < n; i++) doomed[i] = s_old_fabrics[i];
330335
hisense_set_provisioning(false); // paired on the new fabric -> clear "77"
336+
recommission_finish(true, "new fabric committed");
337+
for (uint8_t i = 0; i < n; i++) {
338+
chip::Server::GetInstance().GetFabricTable().Delete(doomed[i]);
339+
}
331340
}
332341
};
333342
static RecommissionFabricDelegate s_recommission_delegate;
@@ -375,8 +384,9 @@ static bool recommission_window_is_open(void)
375384

376385
static bool recommission_grace_expired(void)
377386
{
378-
return (uint32_t) chip::System::SystemClock().GetMonotonicTimestamp().count()
379-
> s_recommission_grace_ms;
387+
// Clock::Timestamp arithmetic, matching s_sync_hold_until above: comparing a truncated
388+
// .count() would be unit-ambiguous and could wrap.
389+
return chip::System::SystemClock().GetMonotonicTimestamp() > s_recommission_grace_until;
380390
}
381391

382392
static void recommission_user_cancel(intptr_t)
@@ -433,8 +443,7 @@ static void recommission_open_window(intptr_t)
433443
s_recommission_pending = true;
434444
chip::DeviceLayer::SystemLayer().StartTimer(chip::System::Clock::Seconds32(kRecommissionWindowSec),
435445
recommission_timeout, nullptr);
436-
s_recommission_grace_ms = (uint32_t) chip::System::SystemClock().GetMonotonicTimestamp().count()
437-
+ kRecommissionGraceMs;
446+
s_recommission_grace_until = chip::System::SystemClock().GetMonotonicTimestamp() + kRecommissionGrace;
438447
hisense_set_provisioning(true); // report prov=1 -> A/C lights "77" while the window is open
439448
ChipLogProgress(DeviceLayer, "recommission: window open %lus, snapshot %u old fabric(s)",
440449
(unsigned long) kRecommissionWindowSec, s_old_fabric_count);

firmware/src/version.txt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
1.3.2
1+
1.3.3

0 commit comments

Comments
 (0)