mirror of
https://github.com/mikecarper/MeshCore.git
synced 2026-10-23 08:43:48 +00:00
fix(display): harden CHSC6X polling and long-gap activity accounting
Addresses a Codex review of the preceding three commits. Touch could disable itself for the whole session. begin() latched _present from a single address probe, but this controller NACKs whenever it has nothing to report, so an idle probe at boot was indistinguishable from absent hardware. The probe is now diagnostics only; checkTap() polls regardless, and a NACK costs one quiet bus cycle. Touch no longer mutates shared bus state. Wrapping the read in setTimeOut()/restore was an unsynchronised global write, and the MQTT task drives the same Wire through AutoDiscoverRTCClock in its NTP fallback, so it could inherit the short timeout. The address probe alone removes the ESP_ERR_TIMEOUT stalls that motivated it, and it also ran before the timeout was installed, so the transaction most exposed to a wedged bus was unprotected anyway. A single failed read could fake a release. The debounce window was 40 ms against a 50 ms poll, so a state change was confirmed by the very next sample and one NACK mid-touch produced a release followed by a second tap. It is now 80 ms - two consecutive consistent reads. RadioActivityWindow froze after a gap longer than ~24.8 days. tick() treated any delta past the signed halfway mark as an out-of-order timestamp, so a node left unserviced that long kept a month-old packet in the 20-minute window and reported it as recently received. A backwards step is now only believed when it is small, which is what an out-of-order reading between two call sites actually looks like. Not changed: a downgrade that then saves prefs drops the display group, since /mqtt.json is rewritten from the older serializer's known schema. That is inherent to every appended field in this format, and bumping the JSON version would be worse - older firmware would reject the file rather than ignore one group. Documented instead.
This commit is contained in:
1 parent
f3c81b559b
commit
018d68063f
5 files changed
+75
-31
No files matched your search
@@ -22,6 +22,10 @@
|
||||
// as a stale or (after a 49-day rollover) nonsensical age.
|
||||
#define RADIO_ACTIVITY_MAX_AGE_MS (100UL * RADIO_ACTIVITY_BUCKET_MS)
|
||||
|
||||
// How far a timestamp may run behind the previous one and still count as an
|
||||
// out-of-order reading rather than a very long forward gap.
|
||||
#define RADIO_ACTIVITY_BACKSTEP_TOLERANCE_MS 5000UL
|
||||
|
||||
struct RadioActivitySnapshot {
|
||||
uint32_t packets;
|
||||
uint32_t wire_bytes;
|
||||
@@ -166,16 +170,22 @@ private:
|
||||
uint8_t _head; // ring index of the current minute
|
||||
bool _ever_received;
|
||||
|
||||
// Accumulates the delta since the previous call, which is correct across one
|
||||
// millis() wrap. Read as signed so a caller handing back a slightly older
|
||||
// reading counts as no time passing, rather than as a ~49-day leap forward
|
||||
// that would expire the whole ring. Successive calls must therefore be less
|
||||
// than 2^31 ms (~24.8 days) apart - guaranteed while the tracker is being
|
||||
// serviced, and when it is not the ring is empty anyway.
|
||||
// Accumulates the unsigned delta since the previous call, which is correct
|
||||
// across one millis() wrap.
|
||||
//
|
||||
// A delta past the halfway mark is ambiguous: it is either a slightly stale
|
||||
// reading or a genuine gap of more than ~24.8 days. recordPacket() and
|
||||
// snapshot() sample millis() microseconds apart, so a real stale reading is
|
||||
// tiny - anything larger is treated as the long gap it is, which matters
|
||||
// because rejecting it outright would freeze the ring and leave a
|
||||
// month-old packet looking recently received.
|
||||
void tick(uint32_t now_ms) {
|
||||
int32_t delta = (int32_t)(now_ms - _last_input_ms);
|
||||
if (delta <= 0) return; // stale or repeated reading: no time has passed
|
||||
_now_ms += (uint32_t)delta;
|
||||
uint32_t delta = now_ms - _last_input_ms;
|
||||
if (delta > 0x80000000u &&
|
||||
(uint32_t)(_last_input_ms - now_ms) <= RADIO_ACTIVITY_BACKSTEP_TOLERANCE_MS) {
|
||||
return; // out-of-order reading: no time has passed
|
||||
}
|
||||
_now_ms += delta;
|
||||
_last_input_ms = now_ms;
|
||||
}
|
||||
|
||||
|
||||
@@ -19,16 +19,12 @@
|
||||
|
||||
#define CHSC6X_READ_LEN 5
|
||||
|
||||
// Bounds a single read. The shared bus can wedge, and the ESP32 default lets a
|
||||
// failed transfer burn ~1 s inside the UI loop.
|
||||
#ifndef CHSC6X_I2C_TIMEOUT_MS
|
||||
#define CHSC6X_I2C_TIMEOUT_MS 10
|
||||
#endif
|
||||
|
||||
class CHSC6XTouch {
|
||||
public:
|
||||
// Probes the bus. Returns false (and disables itself) when nothing answers,
|
||||
// so a board without the touch panel simply carries on without it.
|
||||
// Probes the bus for diagnostics only. The result must NOT gate polling: this
|
||||
// controller NACKs its address whenever it has nothing to report, so a single
|
||||
// idle probe at boot is indistinguishable from absent hardware. checkTap()
|
||||
// keeps polling either way, and a NACK there costs one quiet, fast bus cycle.
|
||||
bool begin(TwoWire& wire = Wire) {
|
||||
_wire = &wire;
|
||||
_wire->beginTransmission((uint8_t)CHSC6X_I2C_ADDR);
|
||||
@@ -58,8 +54,12 @@ public:
|
||||
|
||||
// True exactly once per new touch.
|
||||
bool checkTap(uint32_t now_ms) {
|
||||
if (!_present) return false;
|
||||
return _detector.update(now_ms, readPressed());
|
||||
bool pressed = readPressed();
|
||||
if (pressed && !_present) {
|
||||
_present = true; // answered late; the boot probe caught it mid-idle
|
||||
Serial.println("Touch: CHSC6X responding");
|
||||
}
|
||||
return _detector.update(now_ms, pressed);
|
||||
}
|
||||
|
||||
private:
|
||||
@@ -75,15 +75,12 @@ private:
|
||||
_wire->beginTransmission((uint8_t)CHSC6X_I2C_ADDR);
|
||||
if (_wire->endTransmission() != 0) return false;
|
||||
|
||||
const uint16_t prev_timeout = _wire->getTimeOut();
|
||||
_wire->setTimeOut(CHSC6X_I2C_TIMEOUT_MS);
|
||||
uint8_t got = _wire->requestFrom((uint8_t)CHSC6X_I2C_ADDR, (uint8_t)CHSC6X_READ_LEN);
|
||||
uint8_t buf[CHSC6X_READ_LEN];
|
||||
for (uint8_t i = 0; i < CHSC6X_READ_LEN; i++) {
|
||||
buf[i] = i < got ? (uint8_t)_wire->read() : 0xFF;
|
||||
}
|
||||
while (_wire->available()) _wire->read(); // drain a short read
|
||||
_wire->setTimeOut(prev_timeout);
|
||||
|
||||
if (got != CHSC6X_READ_LEN) {
|
||||
logRaw(got, NULL);
|
||||
|
||||
@@ -9,8 +9,12 @@
|
||||
// elapsed-time comparisons are unsigned subtractions, so millis() rollover is
|
||||
// a non-event.
|
||||
|
||||
// Must exceed the caller's poll interval, or a state change is confirmed by the
|
||||
// very next sample and the debounce does nothing. At the 50 ms touch poll this
|
||||
// requires two consecutive consistent reads, so one NACK or short read during a
|
||||
// continuous touch cannot fake a release (and therefore a second tap).
|
||||
#ifndef TOUCH_TAP_DEBOUNCE_MS
|
||||
#define TOUCH_TAP_DEBOUNCE_MS 40
|
||||
#define TOUCH_TAP_DEBOUNCE_MS 80
|
||||
#endif
|
||||
|
||||
// Ignores a second tap arriving this soon after an accepted one, so a bouncy
|
||||
|
||||
@@ -355,6 +355,23 @@ TEST(RadioActivityWindow, AnOlderTimestampDoesNotExpireTheWindow) {
|
||||
EXPECT_EQ(10000u, s.window_ms);
|
||||
}
|
||||
|
||||
TEST(RadioActivityWindow, AGapLongerThanHalfTheMillisRangeStillExpiresTheRing) {
|
||||
// Display off and no traffic for ~25 days: the elapsed time passes the signed
|
||||
// halfway mark, which must not be mistaken for an out-of-order reading, or a
|
||||
// month-old packet would still be sitting in the "last 20 minutes".
|
||||
RadioActivityWindow w;
|
||||
w.reset(0);
|
||||
recordTypical(w, 1000);
|
||||
ASSERT_EQ(1u, snapshotAt(w, 2000).packets);
|
||||
|
||||
const uint32_t twenty_five_days = 25UL * 24 * 3600 * 1000;
|
||||
ASSERT_GT(twenty_five_days, 0x80000000u) << "gap must cross the halfway mark";
|
||||
|
||||
RadioActivitySnapshot s = snapshotAt(w, twenty_five_days);
|
||||
EXPECT_TRUE(s.isEmpty());
|
||||
EXPECT_FALSE(s.has_last_packet);
|
||||
}
|
||||
|
||||
TEST(RadioActivityWindow, AveragesHandleNegativeSnrAndMixedSigns) {
|
||||
RadioActivityWindow w;
|
||||
w.reset(0);
|
||||
|
||||
@@ -43,7 +43,7 @@ TEST(TouchTapDetector, HoldingAFingerDownDoesNotRepeat) {
|
||||
uint32_t now = 1000;
|
||||
d.reset(now);
|
||||
|
||||
EXPECT_EQ(1, hold(d, now, true, 100));
|
||||
EXPECT_EQ(1, hold(d, now, true, 200));
|
||||
EXPECT_EQ(0, hold(d, now, true, 10000)) << "a long press must not toggle repeatedly";
|
||||
}
|
||||
|
||||
@@ -79,9 +79,9 @@ TEST(TouchTapDetector, SecondTapTooSoonIsSuppressed) {
|
||||
uint32_t now = 1000;
|
||||
d.reset(now);
|
||||
|
||||
EXPECT_EQ(1, hold(d, now, true, 100));
|
||||
EXPECT_EQ(0, hold(d, now, false, 100));
|
||||
EXPECT_EQ(0, hold(d, now, true, 100)) << "inside TOUCH_TAP_MIN_GAP_MS";
|
||||
EXPECT_EQ(1, hold(d, now, true, 200));
|
||||
EXPECT_EQ(0, hold(d, now, false, 150)); // release long enough to be confirmed
|
||||
EXPECT_EQ(0, hold(d, now, true, 200)) << "inside TOUCH_TAP_MIN_GAP_MS";
|
||||
}
|
||||
|
||||
TEST(TouchTapDetector, DeliberateSecondTapIsAccepted) {
|
||||
@@ -89,9 +89,25 @@ TEST(TouchTapDetector, DeliberateSecondTapIsAccepted) {
|
||||
uint32_t now = 1000;
|
||||
d.reset(now);
|
||||
|
||||
EXPECT_EQ(1, hold(d, now, true, 100));
|
||||
EXPECT_EQ(1, hold(d, now, true, 200));
|
||||
EXPECT_EQ(0, hold(d, now, false, 600)); // past the min gap
|
||||
EXPECT_EQ(1, hold(d, now, true, 100));
|
||||
EXPECT_EQ(1, hold(d, now, true, 200));
|
||||
}
|
||||
|
||||
TEST(TouchTapDetector, ASingleBadSampleDuringATouchDoesNotFakeARelease) {
|
||||
// A NACK or short read reports "not pressed" for that poll. One of those must
|
||||
// not confirm a release, or recovery reads as a second tap and the display
|
||||
// toggles twice on one touch.
|
||||
TouchTapDetector d;
|
||||
uint32_t now = 1000;
|
||||
d.reset(now);
|
||||
EXPECT_EQ(1, hold(d, now, true, 200));
|
||||
|
||||
EXPECT_FALSE(d.update(now, false));
|
||||
now += POLL;
|
||||
EXPECT_TRUE(d.isTouched()) << "one bad sample must not confirm a release";
|
||||
|
||||
EXPECT_EQ(0, hold(d, now, true, 2000)) << "and must not produce a second tap";
|
||||
}
|
||||
|
||||
TEST(TouchTapDetector, SurvivesMillisRollover) {
|
||||
@@ -100,10 +116,10 @@ TEST(TouchTapDetector, SurvivesMillisRollover) {
|
||||
uint32_t now = start;
|
||||
d.reset(now);
|
||||
|
||||
EXPECT_EQ(1, hold(d, now, true, 200)); // the touch itself crosses the wrap
|
||||
EXPECT_EQ(1, hold(d, now, true, 250)); // the touch itself crosses the wrap
|
||||
ASSERT_LT(now, start) << "test setup must actually wrap";
|
||||
EXPECT_EQ(0, hold(d, now, false, 600));
|
||||
EXPECT_EQ(1, hold(d, now, true, 200));
|
||||
EXPECT_EQ(1, hold(d, now, true, 250));
|
||||
}
|
||||
|
||||
TEST(TouchTapDetector, ResetClearsPendingState) {
|
||||
|
||||
Reference in new issue
Block a user