From 968a31f963652add60b401c41504b3fb43b16ffd Mon Sep 17 00:00:00 2001 From: cjkas Date: Wed, 21 Oct 2026 13:27:49 +0200 Subject: [PATCH] docs: move open known issues to GitHub, drop the finished plans KNOWN_ISSUES.md loses the fixed entries, and the open ones that are worth fixing are now issues #77-#86, listed at the top. The three finished plans go: the LoopQueue decisions nothing else recorded move into ADR-0002's consequences, the shade behaviour plan's open items become #85 and #86, and the comments that pointed at the plans now say what they meant. Co-Authored-By: Claude Opus 5.5 --- docs/KNOWN_ISSUES.md | 274 ++---------------- docs/LOOP_QUEUE_PLAN.md | 224 -------------- docs/SHADE_BEHAVIOUR_PLAN.md | 169 ----------- docs/SOMFY_SPLIT_PLAN.md | 116 -------- ...k-is-the-only-writer-of-the-shade-model.md | 6 +- src/SomfyJson.cpp | 3 +- .../test_shadebehaviour.cpp | 2 +- 7 files changed, 27 insertions(+), 767 deletions(-) delete mode 100644 docs/LOOP_QUEUE_PLAN.md delete mode 100644 docs/SHADE_BEHAVIOUR_PLAN.md delete mode 100644 docs/SOMFY_SPLIT_PLAN.md diff --git a/docs/KNOWN_ISSUES.md b/docs/KNOWN_ISSUES.md index ed15935..1472703 100644 --- a/docs/KNOWN_ISSUES.md +++ b/docs/KNOWN_ISSUES.md @@ -1,255 +1,26 @@ # Known Issues -## Concurrent mutation during chunked `/controller` response +## Tracked as GitHub issues -**Status:** fixed: reads in step 2, edits in step 3; command replies still read the live model (below). -**Introduced:** 2026-04-19, alongside the chunked-response conversion of `/controller` (the original crash report was about `cbuf::resize` aborting `async_tcp`). -**Fixed:** 2026-10-16, steps 2 and 3 of [LOOP_QUEUE_PLAN.md](LOOP_QUEUE_PLAN.md). +- [#77](https://github.com/cjkas/ESPSomfy-RTS/issues/77) Port 80 routes don't check login +- [#78](https://github.com/cjkas/ESPSomfy-RTS/issues/78) CORS allows any origin +- [#79](https://github.com/cjkas/ESPSomfy-RTS/issues/79) Websocket has no auth +- [#80](https://github.com/cjkas/ESPSomfy-RTS/issues/80) API token key is public +- [#81](https://github.com/cjkas/ESPSomfy-RTS/issues/81) `/login` has no rate limit +- [#82](https://github.com/cjkas/ESPSomfy-RTS/issues/82) A saved WiFi passphrase or MQTT password can't be cleared from the UI +- [#83](https://github.com/cjkas/ESPSomfy-RTS/issues/83) Installing "Main" from the update dialog always fails +- [#84](https://github.com/cjkas/ESPSomfy-RTS/issues/84) Transmitting a frame stops `loop()` for about 365 ms +- [#85](https://github.com/cjkas/ESPSomfy-RTS/issues/85) Integrated tilt turns its slats back with an Up after a lift-only target +- [#86](https://github.com/cjkas/ESPSomfy-RTS/issues/86) ShadeBehaviour: self-echo, group commands and `lastFrame` (structural follow-ups) -### What it was +## Command replies read the live shade model -The chunked `/controller` response read `somfy.rooms`, `somfy.shades`, `somfy.groups` and `somfy.repeaters` on async_tcp a piece at a time as the client's TCP window opened, for hundreds of ms to seconds. Anything that changed them meanwhile could make the document contradict itself: a deleted shade in `shades` but not in a group's `linkedShades`, two positions for one shade, a linked shade skipped or written twice. `/discovery`, `/shades` and the other reads had the same exposure, only shorter. - -All eight read routes (`/controller`, `/discovery`, `/rooms`, `/shades`, `/groups`, `/room`, `/shade`, `/group`) now stream a ModelSnapshot that `loop()` fills, so each document describes one moment ([ADR-0002](adr/0002-loop-task-is-the-only-writer-of-the-shade-model.md)). That is fix option (c), taken per read in a heap slot of about 5.6 KB. - -### Edits (step 3) - -The handlers that changed the shade model on the web server task (adding, saving and deleting rooms, shades and groups; linking remotes, repeaters and group members; `/setPaired`; the sort orders) now hand the edit to `loop()` through the LoopQueue, and every client learns the result from a `requestResult` socket event. So `/linkRemote` can no longer race a received frame for a linked remote's rolling code: both now change it on `loop()`, in the order they happen. - -### Still open - -- Command replies (`/shadeCommand`, `/tiltCommand`, `/unpairShade`, `/setRollingCode`, ...) still copy the one item they report from the live shade model on async_tcp, so a reply can show a value from the middle of a change. `/room`, `/shade`, `/group` and `/groupOptions` also check on the live model whether the item exists, because the status goes out before the snapshot is filled. -- The read routes would leave out an item that didn't fit their 3 KB unit; a test now proves none can, see [Oversized read items are left out](#oversized-read-items-are-left-out). - -## Oversized read items are left out - -**Status:** guarded by a test (fix option (b)). It can't happen with today's fields, and a change that would make it happen fails `test_snapshot_limits`. -**Location:** `SnapshotWriter::overflowed` in [src/ModelSnapshot.cpp](../src/ModelSnapshot.cpp); the unit is `SNAPSHOT_UNIT_SIZE` in [src/ModelSnapshot.h](../src/ModelSnapshot.h). +**Status:** open. What's left of the concurrent-mutation problem that [ADR-0002](adr/0002-loop-task-is-the-only-writer-of-the-shade-model.md) fixed for reads and edits. +**Location:** the command handlers in [src/entryPoints/Web.cpp](../src/entryPoints/Web.cpp). ### Summary -Every read route builds each room, shade, group and linked shade into a 3 KB unit before sending it. When one doesn't fit, the writer logs an `ESP_LOGE` line, skips that item and carries on, so the document stays valid JSON. The client gets no sign that anything is missing: the item just isn't in the list. Before step 2 only `/controller` did this; `/discovery`, `/shades` and the rest grew their buffer instead. - -`test_snapshot_limits` fills a snapshot with the largest possible items: 20-character names of control characters (each escaped to `\u00XX`), all 7 linked remotes, 32 members in every group, and the widest value of every number. It checks that every read document written with the real unit is identical to the same document written with an unlimited one. The largest items are a shade at 1,097 bytes, a linked shade at 345, a group at 333 and a room at 159, against 3,072. The log line stays as a backstop. - -### Concrete failure modes - -1. A shade is left out after a firmware change adds fields to it. It is missing from the config UI, and Home Assistant doesn't create an entity for it. -2. A group whose `linkedShades` entry doesn't fit loses that shade from its list. - -### Fix options (pick one later) - -- **(a) Fail loud.** Add a list of the skipped item ids to the end of the document, and show a warning in the UI. -- **(b) Check the size at build time.** Work out the worst case for each item from the field limits and `static_assert` that it fits the buffer, so a change that breaks it doesn't build. - -### Related - -- The same "lost without telling the client" problem as [Large websocket events are dropped](#large-websocket-events-are-dropped). - -## Large websocket events are dropped - -**Status:** fixed (fix option (c)): the buffer is 4 KB, and the largest event provably fits. -**Location:** [`JsonSockEvent` in src/WResp.cpp](../src/WResp.cpp), buffer `g_response[MAX_SOCK_RESPONSE]` (now 4096 bytes, defined in [src/outputs/SocketEmitter.h](../src/outputs/SocketEmitter.h)). - -### Summary - -Socket events are built into one static buffer. When a value doesn't fit, [`JsonSockEvent::_safecat`](../src/WResp.cpp) marks the event as overflowed, and `endEvent` logs an error and doesn't send it. Clients never see malformed JSON. The event is simply lost, and the client isn't told. - -Unlike the `/controller` HTTP crash, this path does **not** abort: there is no growing cbuf and no `new[]` on the send path. - -### What can be large - -- **`remoteFrame`** (the frame log) lists every pulse of a received frame: up to `MAX_TIMINGS` = 300. A pulse recorded while waiting for a sync has no upper bound, so in theory each can be 10 digits: 300 × 11 bytes plus about 300 bytes of fields is about 3.8 KB. Real frames come to 1.8–2.1 KB, which was already at the old 2 KB limit. A `static_assert` in `Transceiver.cpp` fails the build if `MAX_TIMINGS` or the buffer change so that it no longer fits. -- **The state events are small.** `shadeState` has about 20 fields and no linked remotes, about 400 bytes. (The 1.3–1.5 KB figure this entry used to give was for a shade *with* its linked remotes, which only the read routes send.) `groupState` lists at most 32 shade ids. `frequencyScan`, `memStatus`, `fwStatus`, `ethernet` and `requestResult` have a few fixed fields each. - -The 2 KB more of static RAM is the cost. The log line stays as a backstop. - -### Options that weren't taken - -- **(a) Fail loud:** send a small error event in place of the dropped one so the client reloads the state. -- **(b) Split large events across frames:** the socket.io ack/chunk pattern, with matching client-side reassembly. - -### Related - -- Not the same code path as the `/controller` crash. Solve independently. -- Worth grepping for `JsonSockEvent` usages that iterate collections (see references in `SomfyEvents.cpp`, `Transceiver.cpp`, `ESPNetwork.cpp`, `GitOTA.cpp`) to identify the most at-risk events. -- [Group reads emit events from the web server task](#group-reads-emit-events-from-the-web-server-task) (fixed): a second task writing the same buffer. - -## Group reads emit events from the web server task - -**Status:** fixed 2026-10-16, step 2 of [LOOP_QUEUE_PLAN.md](LOOP_QUEUE_PLAN.md) (fix option (a)). Read routes write from a ModelSnapshot, and the replies still written on async_tcp copy the group with `snapshotGroup()`, which works its flags out without changing the group or sending anything. -**Location:** `serializeGroupRef` in [src/entryPoints/Web.cpp](../src/entryPoints/Web.cpp) called `SomfyGroup::updateFlags()` in [src/SomfyGroup.cpp](../src/SomfyGroup.cpp). - -### Summary - -Serializing a group for an HTTP reply first calls `group->updateFlags()`. When the group's flags no longer match its shades', that writes `group->flags`, sends a `groupState` socket event and publishes the group's MQTT topics, all on the async_tcp task, while `loop()` sends events and publishes too. - -Every socket event is formatted into the one static buffer `g_response` in [src/outputs/SocketEmitter.cpp](../src/outputs/SocketEmitter.cpp) through the one `JsonSockEvent` in `sockEmit`. Two tasks emitting at once write over each other's event. The MQTT client (PubSubClient) isn't safe to use from two tasks either. - -Reached from every route that serializes a group: `/controller`, `/discovery`, `/groups`, `/group`, `/groupCommand`, `/addGroup`, `/saveGroup`, `/linkToGroup`, `/unlinkFromGroup` and others. - -### Concrete failure modes - -1. A `groupState` from a `/groups` request and a `shadeState` from `loop()` are formatted at the same time: clients receive garbled JSON, or an event with another event's fields. -2. The group's MQTT publish interleaves with one from `loop()` in the client's buffer, and the broker gets a broken packet or the connection drops. -3. `group->flags` is written on async_tcp while `loop()` updates the same group through `updateGroupFlags()`. - -It needs a group whose flags are out of date at the moment a client reads it, for example right after a sensor frame changed a linked shade's sun or wind flags, so it is rare. - -### Fix options - -- **(a) Drop `updateFlags()` from the serializers.** `loop()` already keeps group flags current through `updateGroupFlags()`. This is part of step 2 of [LOOP_QUEUE_PLAN.md](LOOP_QUEUE_PLAN.md): reads come from a ModelSnapshot that `loop()` fills, and serializers only read. -- **(b) Only emit from `loop()`.** Covered by ADR-0002 once nothing outside `loop()` changes the shade model. - -### Related - -- [ADR-0002](adr/0002-loop-task-is-the-only-writer-of-the-shade-model.md): the loop task is the only writer of the shade model. -- [Large websocket events are dropped](#large-websocket-events-are-dropped): the same `g_response` buffer. - -## A saved WiFi passphrase or MQTT password can't be cleared from the UI - -**Status:** open. A workaround exists (below). -**Introduced:** 2026-10-18, when the device stopped sending stored secrets back to clients. -**Location:** `applySecret` in [src/Secrets.h](../src/Secrets.h), called from `WifiSettings::fromJSON` and `MQTTSettings::fromJSON` in [src/ConfigSettings.cpp](../src/ConfigSettings.cpp) and from `/connectwifi`. The UI side is `keepSavedPassphrase` in `Wifi` and `connectMQTT` in `MQTT`, in [data-src/index.js](../data-src/index.js). - -### Summary - -The settings routes report only whether a secret is stored (`hasPassphrase`, `hasPassword`), so the UI loads every secret field empty. The firmware decides what an empty secret in a request means, so that every UI version is safe, including one older than this change, which sends `""` for the field it can no longer fill in: - -| Secret | Key absent | `""` | A value | -|---|---|---|---| -| WiFi passphrase | keep | keep, or clear if the same request changes the SSID | replace | -| MQTT password | keep | keep, or clear if the same request changes the username | replace | -| Login password, PIN | keep | keep | replace | - -Without "empty keeps", saving any other setting from an old UI would wipe the secret: for WiFi the device would reboot and drop into AP mode. - -The cost: an empty value can't mean "no password" while the SSID or username stays the same. Switching to a different open network works. - -The login password and PIN aren't affected. Neither can be empty, and security is turned off with the type selector. - -### Concrete failure modes - -1. The network the device is on is changed to open on the router, keeping its SSID. Saving the network page with a blank passphrase keeps the old one. When a passphrase is set, `WiFi.begin` won't accept anything weaker than WPA2-PSK, so the device can't join the now-open network. -2. The MQTT broker stops requiring a password but keeps the username. The device keeps sending the old password. - -### Workaround - -- **MQTT:** save once with a different username, then change it back and save again with the password blank. -- **WiFi:** there's no safe workaround. Saving a different SSID makes the device reboot and try to join that network. - -### Fix options - -- **(a) "Clear saved password" checkbox** next to the WiFi passphrase and MQTT password fields, shown only when a value is stored. Since `""` means keep, ticking it sends an explicit flag (`clearPassphrase` / `clearPassword`), which `fromJSON` turns into `applySecret(..., identityChanged = true)`. - -## Port 80 routes don't check login - -**Status:** open. Fix planned as PR A of [SECURITY_PLAN.md](SECURITY_PLAN.md). -**Location:** `registerWebUiRoutes` in [src/entryPoints/WebUiRoutes.cpp](../src/entryPoints/WebUiRoutes.cpp). - -### Summary - -Only the ten port-80 routes that share a handler with port 8081 call `isAuthenticated`. The other ~60 check nothing, including every settings route, `/restore`, firmware updates, pairing and rolling codes. Logging in only hides parts of the UI. `serveStatic("/")` also serves `shades.cfg` and `shades.tmp`, which hold the rolling codes, to anyone. - -### Concrete failure modes - -1. With a PIN or password set, any LAN client can `PUT /saveSecurity` and replace them, or change the network, MQTT or radio settings. -2. Any LAN client can download `/shades.cfg` and read every shade's remote address and rolling code. - -## API token key is public - -**Status:** open. Fix planned as PR B of [SECURITY_PLAN.md](SECURITY_PLAN.md). -**Location:** `Web::createAPIToken` in [src/entryPoints/Web.cpp](../src/entryPoints/Web.cpp). - -### Summary - -Tokens are `HMAC(serverId, PIN-or-password + client IP)`. `serverId` is the last 3 bytes of the MAC address, and it is published in mDNS, in the retained MQTT `serverId` topic, in `/discovery` and in `/loginContext`. So the HMAC key gives no protection: whoever sees a token can try every PIN offline. Tokens never expire, and they are compared with a non-constant-time `String` comparison. `serverId` itself is an identity (HA's unique id) and stays public. Only the key is replaced. - -## CORS allows any origin - -**Status:** open. Fix planned as PR A of [SECURITY_PLAN.md](SECURITY_PLAN.md). -**Location:** `DefaultHeaders` in `Web::begin`, [src/entryPoints/Web.cpp](../src/entryPoints/Web.cpp). - -### Summary - -Every response carries `Access-Control-Allow-Origin: *`, `Access-Control-Allow-Methods: PUT,POST,GET,OPTIONS` and `Access-Control-Allow-Headers: *`. Any website open in a browser on the LAN can call the device and read the replies. With security None it can change every setting, without the user noticing. - -## Websocket has no auth - -**Status:** open. Fix planned as PR A of [SECURITY_PLAN.md](SECURITY_PLAN.md). -**Location:** `SocketEmitter::wsEvent` in [src/outputs/SocketEmitter.cpp](../src/outputs/SocketEmitter.cpp). - -### Summary - -Anyone can connect to `ws://host:8080/` and join any room. Clients can't change anything over it, but they receive all shade and group state, and room 0 (the frame log) carries received remote addresses and rolling codes. Browsers don't apply CORS to websockets, so a page from any origin can connect too. - -## `/login` has no rate limit - -**Status:** open. Fix planned as PR C of [SECURITY_PLAN.md](SECURITY_PLAN.md). -**Location:** `Web::handleLogin` in [src/entryPoints/Web.cpp](../src/entryPoints/Web.cpp). - -### Summary - -Failed logins aren't throttled. A digits-only 4-character PIN has 10,000 options and falls to an online attack within minutes. An alphanumeric one has about 1.7 million and lasts a day or two at about 10 attempts per second. - -## Boot waits 5 s for the clock and then skips the time zone - -**Status:** fixed (fix option (b)). `apply()` calls `configTzTime(posixZone, ntpServer)`, which sets the zone, starts SNTP and doesn't wait. `save()` calls `apply()`, so saving the NTP settings doesn't wait either. -**Fixed:** 2026-10-19. The same change makes `/setgeneral` save a request that changes only the time zone: it checked `ntpServer` twice instead of `ntpServer` or `posixZone`. -**Location:** `NTPSettings::apply` in [src/ConfigSettings.cpp](../src/ConfigSettings.cpp), called from `ConfigSettings::begin` in `setup()`. - -### What it was - -`apply()` called `configTime()` and then `getLocalTime(&dt)`. Arduino's `getLocalTime` takes a timeout that defaults to 5000 ms, and polls until the year is past 2016. `setup()` calls it before the network is up, so on a boot whose clock isn't set yet it waits the full 5 s and returns false. In the logs, `File system mounted successfully` is followed 5.1 s later by `Launching web server...`. A soft reboot four hours after boot passed straight through in 90 ms, because the RTC keeps the time across a soft reset. One soft reboot two minutes after a cold boot still waited the full 5 s, presumably because SNTP hadn't set the clock yet. - -`apply()` returned as soon as `getLocalTime` failed, before `setenv("TZ", ...)`, and nothing else set `TZ`. So after such a boot the configured time zone wasn't applied: every cold boot was 5 s slower, and local times were worked out as if the zone were UTC. `save()` had the same wait, on the web server task. - -### Options that weren't taken - -- **(a) Don't wait.** `getLocalTime(&dt, 0)`, then set `TZ` whatever the result. - -## A restore reboots even when the backup fails validation - -**Status:** fixed, except that a size mismatch stays a warning (below). -**Fixed:** 2026-10-19. -**Location:** the `/restore` route in [src/entryPoints/WebUiRoutes.cpp](../src/entryPoints/WebUiRoutes.cpp); the `restore_shades` and `load_shades` cases in `LoopQueue::drain` in [src/LoopQueue.cpp](../src/LoopQueue.cpp); `ShadeConfigFile::validate` and `TRANS_REC_SIZE` in [src/ConfigFile.cpp](../src/ConfigFile.cpp). - -### What it was - -The uploaded backup is written to `/shades.tmp`, and `loop()` restored it with `ShadeConfigFile::restore()` and ignored the result. The device rebooted whether the restore worked or not, and the user was never told. Nothing deleted `/shades.tmp` afterwards, so a full copy of the backup, rolling codes included, stayed in LittleFS where `GET /shades.tmp` served it to anyone. - -### What changed - -- The `/restore` reply validates the file first. A bad one gets a `400` with a message, which the restore dialog now shows, and nothing is queued. An upload that didn't complete gets a `400` too; before, it got no reply at all. -- `restoreFile()` only fails while it opens or validates the file, before it changes anything. So when it fails, `loop()` logs it and doesn't reboot. -- `/shades.tmp` is deleted after every restore, and after `/updateShadeConfig` loads it. - -### Why the size check stays a warning - -The backup restored on 2026-10-18 was 6 bytes longer than its header said. Its transceiver record is 80 bytes, but the header declares 74. `TRANS_REC_SIZE` was left at 74 when `noiseDetection` was added to the record, so every backup written since then declares that record 6 bytes short. The reader copes, because `readTransRecord()` skips to the record end. `TRANS_REC_SIZE` is now 80. A strict size check would still reject every backup made before the fix, so a mismatch is only logged, and the record checks catch real damage. The sum lives in `expectedFileBytes()` in [src/ConfigHeader.h](../src/ConfigHeader.h), with tests built from that backup's header. - -The repeater record's length is now checked too, but also only logged: `validate()` runs on `shades.cfg` at every boot, and failing it there would start the device without its shades. - -## Transmitting a frame stops `loop()` for about 365 ms - -**Status:** open, by design for now. Measured in the serial logs from 2026-10-18 and 2026-10-19. -**Location:** `Transceiver::sendFrame` and `startTxTask` in [src/Transceiver.cpp](../src/Transceiver.cpp). - -### Summary - -Every shade command that moves a shade is followed by `Loop queue took 364-370ms` in the log. A command whose shade doesn't need to move takes no time. In a burst of 10 commands from Home Assistant, the four that came after a moving shade waited behind it, and that drain took 426 ms. Once, `Loop somfy took 855ms` came with no command near it. - -The time isn't spent in `loop()`'s own code. The `radio_tx` task sends each frame by toggling the TX pin with `delayMicroseconds()` busy-waits: the wake-up pulse, the syncs, 56 or 80 bits at 1.28 ms each and the gap after each frame. That's about 130 ms for the first frame and about 140 ms for each repeat. The task runs at priority 5, pinned to the same core as `loop()`, which runs at priority 1, so `loop()` doesn't run until the transmission ends. The timing logged for whatever `loop()` was doing includes that wait. - -### Concrete failure modes - -1. While a frame is on air, `loop()` doesn't handle sockets, MQTT or queued commands. -2. A group command, or a burst of shade commands, adds up to several hundred ms for each shade that moves. - -### Fix options - -- **Send with the RMT peripheral.** RMT plays out the pulse train in hardware, so no task has to busy-wait. That means rewriting `sendFrame`, and testing the timing against real motors. -- **Not taken:** a lower priority for `radio_tx` lets `loop()` cut into a frame and breaks its timing. Moving it to the other core holds up WiFi and the web server instead. +The read routes stream a ModelSnapshot that `loop()` fills, and edits go to `loop()` through the LoopQueue ([ADR-0002](adr/0002-loop-task-is-the-only-writer-of-the-shade-model.md)). Command replies (`/shadeCommand`, `/tiltCommand`, `/unpairShade`, `/setRollingCode`, ...) still copy the one item they report from the live shade model on async_tcp, so a reply can show a value from the middle of a change. `/room`, `/shade`, `/group` and `/groupOptions` also check on the live model whether the item exists, because the status goes out before the snapshot is filled. ## A read of `/controller` took 2.4 s @@ -260,17 +31,10 @@ The time isn't spent in `loop()`'s own code. The `radio_tx` task sends each fram On 2026-10-19 the log shows `web GET /controller from 192.168.1.197 total took 2353.274ms (slow)`. That was about 30 s after Home Assistant's socket connected, and 80 s after boot. Every other `/controller` read in the logs took 100-235 ms. -## Log lines that mislead or are missing +## `.gz does not exist` errors are log noise -**Status:** two of three fixed 2026-10-19. The `.gz` noise is still open. All cosmetic. +**Status:** open. Cosmetic. +**Location:** `serveStatic` in [src/entryPoints/WebUiRoutes.cpp](../src/entryPoints/WebUiRoutes.cpp); `MINIFY_AND_GZIP` in [minify.py](../minify.py). -- **Fixed: a command for an unknown shade left no trace.** `handleShadeCommand`, `handleTiltCommand` and `handleGroupCommand` in [src/entryPoints/Web.cpp](../src/entryPoints/Web.cpp) now log a `not found` warning. On 2026-10-18 Home Assistant sent 10 commands to a device that had no shades yet, and the log showed only the response times. -- **Fixed: `last ping 0ms ago` meant the client never pinged.** The disconnect line in [src/outputs/SocketEmitter.cpp](../src/outputs/SocketEmitter.cpp) now says `never pinged`. Browsers don't ping, so they always showed it. -- **Open: `.gz does not exist` errors are noise.** `serveStatic` tries `.gz` first. For files stored without one (`appversion`, `icon.png`), LittleFS logs an `E`-level `does not exist, no permits for creation` line on every request. The fix would be for `minify.py` to gzip the plain files too. `setTryGzipFirst(false)` doesn't help: the error comes from `exists()` probing the missing variant, so it would only move the noise to the gzip-only files. +`serveStatic` tries `.gz` first. For files stored without one (`appversion`, `icon.png`), LittleFS logs an `E`-level `does not exist, no permits for creation` line on every request. The fix would be for `minify.py` to gzip the plain files too; `appversion` must stay plain as well, because `ConfigSettings` reads it from LittleFS. `setTryGzipFirst(false)` doesn't help: the error comes from `exists()` probing the missing variant, so it would only move the noise to the gzip-only files. -## Installing "Main" from the update dialog always fails - -**Status:** open. Found 2026-10-20 while reviewing PR #72. -**Location:** `GitUpdater::beginUpdate` in [src/GitOTA.cpp](../src/GitOTA.cpp). - -The update dialog offers a "Main" entry next to the releases. For it, the updater downloads from `https://raw.githubusercontent.com/cjkas/ESPSomfy-RTS/master/`, but the repository holds no `.bin` files, so the firmware download 404s. Nothing is written and the device keeps running, so it does no harm, but the option can't work. Either drop the entry or point it at a build that publishes per-chip images. diff --git a/docs/LOOP_QUEUE_PLAN.md b/docs/LOOP_QUEUE_PLAN.md deleted file mode 100644 index 29defcc..0000000 --- a/docs/LOOP_QUEUE_PLAN.md +++ /dev/null @@ -1,224 +0,0 @@ -# Plan: domain modules behind the LoopQueue - -**Status:** all steps built (PRs #39 to #44). -**Written:** 2026-10-15, against commit `e624a41` on `refactor/entry-points-outputs`. Terms are defined in [GLOSSARY.md](../GLOSSARY.md); the threading rule is [ADR-0002](adr/0002-loop-task-is-the-only-writer-of-the-shade-model.md). - -## Why - -- **Two translators for the same requests.** HTTP (`Web::processQueue`) and MQTT (`MQTTClass::receive`) each turn a request into shade calls, and they have drifted apart: - - MQTT `tiltTarget`, `mypos` and `tiltPosition` ignore `flipPosition`; - - `/setPositions` ignores it the other way round; - - `repeat=0` means "default" for shades but "no repeats" for groups. -- **Writers on the web task.** Model edits (`/addShade`, `/linkRemote`, `/setPaired`, ...) and every read (`/controller`, `/shades`, `/discovery`, ...) touch the shade model on the async_tcp task while `loop()` changes it. See [KNOWN_ISSUES.md](KNOWN_ISSUES.md) and ADR-0002. -- **Publishing depends on discipline.** Each `p_*` setter publishes its own MQTT topic, and `emitState()` calls are placed by hand. Writes that skip a setter are never published, and MQTT `position` has been a constant 50 since `c94819e`. -- **No native tests.** None of this compiles in the native env, so it has no tests. - -## Shape - -``` -WebUiRoutes ─┐ ┌─► ShadeRequests ─► shades/groups ─► radio -ApiRoutes ──┼─► LoopQueue ─► loop() ┼─► ModelEditor ─► shade model -MqttCommands ┘ (one FIFO) └─► ModelSnapshot fill ─► read reply (chunked) - │ dirty bits, once per loop pass - ▼ - MqttPublisher · SocketEmitter -``` - -| Module | Covers | Called by | -|---|---|---| -| **ShadeRequests** | shade and group commands; favorite and pairing; position and rolling-code correction | WebUiRoutes, ApiRoutes, MqttCommands | -| **ModelEditor** | add, save, delete, link and sort rooms, shades, groups and repeaters | WebUiRoutes | -| **ModelSnapshot** | the eight read routes: `/discovery` `/controller` `/rooms` `/shades` `/groups` `/room` `/shade` `/group` | WebUiRoutes, ApiRoutes | - -**Settings, network, radio config, firmware and discovery** have one transport each and little logic of their own. They keep calling the modules they use today (`ConfigSettings`, `ESPNetwork`, `Transceiver`, `GitOTA`, `SSDP`); a new module would only pass calls through. The exception is pure validation, such as the pin-conflict check, which moves into a function the native tests can reach. - -## Decisions - -### LoopQueue -- One FIFO from every other task into `loop()`. It carries shade requests, model edits and snapshot fills. -- No item jumps the queue, model edits included. -- MQTT goes through it too, although it already runs on `loop()`. That gives MQTT the radio back-pressure that web commands have (`holdWebCommand`) and keeps arrival order. - -### ShadeRequests -- **Interface:** one request value, grown out of `web_command_t`. Positions are given as the client sees them. -- **It owns:** - - looking up the shade or group; - - flipping positions; - - range checks; - - repeat defaults; - - refusing a request when the radio can't take it. -- **Test seam:** the shade sits behind exactly the operations `processQueue` calls today: - - `moveToTarget`, `moveToTiltTarget`; - - `sendCommand`, `sendTiltCommand`, `repeatFrame`/`isLastCommand`, `sendSensorCommand`; - - `setMyPosition`, unpair, `setRollingCode`, set positions; - - a lookup giving the shade's or group's existence, flip and tilt type. -- **Two implementations behind it:** the real one forwards to `SomfyShade`/`SomfyGroup`; a fake records the calls in native tests. -- **Later:** when the shade behaviour itself is deepened (candidate #1 of the architecture review), only the real implementation changes. - -### ModelEditor -- **What it takes over** from the `Web.cpp` handlers and `SomfyShadeController`: - - id and sort-order allocation (the six `getNext*Id`/`getMax*Order` copies); - - limits; - - validation; - - linking rules: remote to shade, shade to group, repeaters; - - what is committed and which events go out after each edit. -- **What `SomfyShadeController` keeps:** storage and lookup. -- **Request bodies:** - - The handler copies the JSON body to the heap and queues a pointer to it. - - `loop()` parses and validates it with the code in `SomfyJson.cpp`; keeping it there avoids a second ArduinoJson file. - - At most one edit is in flight; another gets 503. A body over 4 KB gets 413. -- **Reply and result:** - - The HTTP reply is `{status: "accepted", requestId}`. - - Once `loop()` has made the change, it sends the existing `…Added`/`…Removed`/`…State` events, with their payloads unchanged (Home Assistant reads `shadeRemoved`). - - It also broadcasts `requestResult {requestId, ok, desc, id}`. The UI that sent the request picks it out by `requestId`. -- **Web UI:** - - It fetches the details it needs through a read route, for example `/shade?shadeId=` after an add. - - If no `requestResult` arrives within about 5 s, it shows a popup that says the change wasn't confirmed and offers a reload. -- **Socket-handler fixes** in `index.js`, part of this step: - - `shadeAdded`/`shadeRemoved` are no-ops; - - `procRoomRemoved` keeps the removed room; - - the reply body is read even on errors. - -### ModelSnapshot -- **One snapshot type** with the reported fields of rooms, shades, groups and repeaters. That is about 5.5 KB at 32 shades, 16 groups and 16 rooms, against roughly 24 KB for a copy of the full `SomfyShade` objects. -- **One snapshot per read:** - 1. The handler allocates the snapshot (one item for `/shade`, `/group` and `/room`) and queues a fill. - 2. The chunked response returns `RESPONSE_TRY_AGAIN` until `loop()` has filled the snapshot. - 3. It then streams JSON from that private copy. -- **Not in the snapshot:** the parts outside the shade model are built on the fly. That covers the `/discovery` device info and memory figures, and the `/controller` header (limits, radio config, firmware version). -- **Limit:** at most 2 reads in flight; a third gets 503. -- **Serializers** work from the snapshot, not from `somfy`. They lose `group->updateFlags()`, which sends events from the web task today, and they become testable natively through `JsonFormatter`. - -### Change fan-out -- **Marking:** - - Each shade and group gets a bitmask with one bit per reported value: position, target, direction, tilt, favorite, sun flags, rolling code, and so on. - - The `p_*` setters set the bit, and only when the displayed value changes, as today. -- **Sending:** once per `loop()` pass, for each marked shade or group: - - the socket gets one `shadeState`/`groupState`; - - MQTT gets only the topics whose bits are set; - - then the bits are cleared. -- **Reconnects:** what an output misses while disconnected comes back on reconnect. MQTT `connect()` publishes everything, and a new socket client gets `initClients()`. -- **Events that aren't state** (`shadeCommand`, "remote X pressed Up") stay explicit calls. - -### Flash -- **Debug build:** `esp32devdbg` moves to `esp32_huge_app.csv` (one 3 MB app, no OTA) and stays there. It isn't shipped, and at 98.3% of the OTA slot it had 26 KB left. -- **Release builds:** the release envs keep their partition tables, and CI builds them. `esp32dev` is at 1,396,957 bytes (88.8%), so 176 KB is free. -- **Firmware-update testing** has to use a release build. - -## Steps - -Each step is a PR of its own that can be tested on the device. Each one runs `pio test -e native` and the `esp32devdbg` and `esp32dev` builds. - -| # | Step | Closes | -|---|---|---| -| 0 | Move `esp32devdbg` to `esp32_huge_app.csv`, in a commit of its own. The device then needs one USB flash (see below). | flash pressure during the refactor | -| 1 | LoopQueue + ShadeRequests. Web and MQTT both submit shade requests. Native tests run through the fake shade. | MQTT flip drift, repeat defaults, MQTT commands lost while the radio is full | -| 2 | ModelSnapshot for all eight read routes. Native tests cover the serializers. | inconsistent `/controller` documents, events sent from the web task | -| 3 | ModelEditor, `requestResult` and the `index.js` changes. | the rest of the web-task writers in KNOWN_ISSUES | -| 4 | Change fan-out with dirty bits, replacing publishing from the `p_*` setters. | writes that skip MQTT, `position` stuck at 50 | -| 5 | Split `Web.cpp` into WebUiRoutes and ApiRoutes, and `MQTT.cpp`/`SomfyMqtt.cpp` into MqttCommands, an MQTT connection and MqttPublisher. Edit parsing stays in `SomfyJson.cpp`. | the glossary names land in the code | - -### Step 1: what changed in behaviour -- **Flip handling is the same for every entry point.** MQTT `tiltTarget`, `mypos`, `myTiltPos` and `tiltPosition`, and `/setPositions` (which Home Assistant calls), now turn the position round for a shade with `flipPosition`, as the other routes always did. -- **Setting the favorite over MQTT keeps the other half.** `mypos` on a shade with tilt keeps the favorite tilt; before, it set it to 0. `myTiltPos` keeps the favorite lift, or uses where the shade is when it has none. -- **`repeat` means the same everywhere.** Left out or negative is the shade's or group's own repeats; 0 is no repeats. `/shadeCommand` used to read 0 as "the shade's own". A JSON `repeat` over 127 is capped; it used to wrap round to "the shade's own". -- **Position corrections reach MQTT.** `/setPositions` and MQTT `position`/`tiltPosition` go through the setters, so they are published. -- **MQTT commands go through the LoopQueue.** While the radio is full they wait instead of being refused, and they run in order with web requests. An MQTT command topic that isn't understood is logged. -- **The LoopQueue is drained on every `loop()` pass**, with or without a network. It is no longer drained from inside a GitHub firmware download, so a restore can't reboot the device in the middle of one. - -`WebCommand.h` became `LoopQueue.h` (the item and the hold rule) and `ShadeRequests.h` (the request, the `ShadeOps` seam and the module). `SomfyShadeOps` is the device side of the seam. `entryPoints/MqttCommands` holds the topic parsing. There are 45 new native tests (271 in all). - -Flash: `esp32devdbg` 1,546,405 → 1,547,381 bytes, `esp32dev` 1,396,957 → 1,398,337 bytes. - -### Step 2: what changed in behaviour -- **Each read describes one moment.** `/controller`, `/discovery`, `/rooms`, `/shades`, `/groups`, `/room`, `/shade` and `/group` stream a ModelSnapshot that `loop()` fills. The first two KNOWN_ISSUES items about `/controller` are fixed for reads. -- **Every read is chunked.** `/discovery`, `/shades` and the rest used to be built whole into a growing response buffer, the path of the old `cbuf::resize` crash. -- **A read waits for `loop()`.** The headers go out at once; the body follows once the snapshot is filled. That is usually one `loop()` pass, but if the headers' ACK comes back first the library only asks again on its next poll, up to about 500 ms later. A read queued behind a command that waits for the radio waits with it. -- **At most two reads at once.** A third gets 503 "Busy, try the read again". Each read holds a 5.6 KB snapshot and a 3 KB unit on the heap only while it runs. -- **An item too big for the 3 KB unit is left out of every read route**, not only `/controller` (see KNOWN_ISSUES). -- **`/room`, `/shade` and `/group`** still answer 500 for an id that doesn't exist. If the item disappears between that check and the snapshot, the reply is 200 with `{"status":"ERROR","desc":"… Id not found."}`. -- **Group flags in replies are worked out without side effects.** Serializing a group no longer calls `updateFlags()`, which sent `groupState` and published MQTT from the web server task. -- **`/discovery` no longer logs where its time went.** Building it is now spread over the chunks. - -`ModelSnapshot.h/.cpp` holds the snapshot types, the JSON for each item, `SnapshotWriter` (the eight documents, built a unit at a time) and `SnapshotSlots`. `SomfySnapshot.cpp` copies the shade model into it, for `loop()`'s fills and for the replies still written on the web server task. The `SOMFY_MAX_*` limits moved to `ShadeTypes.h` so the native tests can use them. 21 new native tests, 292 in all. - -Flash: `esp32devdbg` 1,547,381 → 1,547,821 bytes, `esp32dev` 1,398,337 → 1,400,401 bytes. - -### Step 3: what changed in behaviour -- **Every edit of the shade model runs on `loop()`.** The 19 edit routes reply `{"status":"accepted","requestId":N}` and queue a copy of the request. `loop()` applies it and broadcasts `requestResult {requestId, ok, id, code, desc}`. The errors that used to come in the HTTP reply ("Shade Id not found.", "Data Error." with its code, and so on) come in `requestResult` instead. -- **One edit at a time.** A second edit gets 503 "Busy, try the change again", and the web UI retries it. A body over 4 KB gets 413. -- **The web UI waits for the result** and then reads the item back (`/shade`, `/group`, `/room`, `/repeaters`), so the code that used the old reply body is unchanged. If no result comes within 5 s, it offers to reload the page. Reads that get 503 "Busy" are retried too. -- **Added, removed and saved items reach every client.** Saving sends `shadeState`, `groupState` or `roomState`. The web UI now acts on `shadeAdded`, `shadeRemoved`, `groupAdded` and `groupRemoved` from other clients. -- **A shade that fails validation is no longer added.** `/addShade` used to keep the new shade with defaults when its pins clashed. -- **Sort orders and linked repeaters are saved.** Linking a remote, a repeater or a group member reports when the list is full. -- **`/getNextRoom`, `/getNextShade`, `/getNextGroup`, `/groupOptions` and the new `/repeaters`** are snapshot reads. -- **ModelEditor's rules** (`ModelEditor.h/.cpp`) replace the six `getNext*Id`/`getMax*Order` copies, the remote-address search, the repeater and group-member list code, and the pin-conflict check. The pin check now uses the protocol being saved and checks every pin against Ethernet. - -`SomfyEditor.h` declares the edit handed to `loop()`. The code that applies it lives in `SomfyJson.cpp`, next to `fromJSON`, so ArduinoJson stays in one file. 27 new native tests, 319 in all. - -Flash: `esp32devdbg` 1,547,821 → 1,536,337 bytes, `esp32dev` 1,400,401 → 1,393,221 bytes. The 19 handler bodies became one path. - -### Step 4: what changed in behaviour -- **MQTT gets the shade's real `position` and `target`.** Both were a constant 50 since `c94819e`, and `direction` was a constant 0 when MQTT connected. -- **Each pass sends a changed shade or group once.** The `p_*` setters and `emitState()` only mark the shade (`SHADE_CHANGED_*` bits in `ShadeChanges.h`). At the end of each `loop()` pass, `SomfyShadeController::sendChanges()` publishes the changed MQTT topics and sends one `shadeState` or `groupState`. - - A group whose flags change now sends one `groupState`, not two. - - `emitState(evt)` with an event name (`shadeAdded`, ...) and `emitState(num)` for one client still go out at once. -- **`position` and `tiltPosition` are retained by the broker** when they change, as they already were when MQTT connected. A client that subscribes later gets the current position. -- **Topic values come from one tested place.** `publishShadeTopics()` writes the topics from a shade snapshot, and `publishState()` (all topics, on connect) uses it too. - -8 new native tests, 327 in all. Flash: `esp32devdbg` 1,536,337 → 1,536,533 bytes, `esp32dev` 1,393,221 → 1,393,449 bytes. - -### Step 5: the file split -Code moved unchanged; only files and names changed. - -| File | Holds | -|---|---| -| `entryPoints/WebUiRoutes.cpp` | `registerWebUiRoutes()`: the port 80 routes and the UI's files, with the helpers only they use | -| `entryPoints/ApiRoutes.cpp` | `registerApiRoutes()`: the port 8081 routes | -| `entryPoints/Web.cpp` | the web servers' lifecycle, auth, and the handlers both route sets call | -| `entryPoints/WebRoutes.h` | what those three share | -| `entryPoints/MqttCommands.*` | + `MQTT_COMMAND_TOPICS`, the topics the connection subscribes to | -| `MqttConnection.*` | was `entryPoints/MQTT.*`; class `MQTTClass` → `MqttConnection` | -| `outputs/MqttPublisher.cpp` | was `outputs/SomfyMqtt.cpp` | -| `outputs/SocketEmitter.*` | was `outputs/Sockets.*` | - -The subscribe and unsubscribe lists, which repeated `groups/+/direction/set`, became the one `MQTT_COMMAND_TOPICS` list, and a native test checks that the parser takes every topic in it. Flash: `esp32devdbg` 1,536,533 → 1,539,645 bytes (+3.1 KB, since the routes are a second file that uses ArduinoJson, as SOMFY_SPLIT_PLAN.md warned), `esp32dev` 1,393,449 → 1,391,113 bytes (−2.3 KB, where `-Os` inlines less). - -### Step 0 on a device -**No longer needed:** `esp32_huge_app.csv` now has the release file system (`0xE0000` at `0x310000`) and puts the coredump in the 64 KB that was unused at the end, so the debug and release builds share one data layout and either can be flashed over the other. A device formatted at `0xD0000` grows to `0xE0000` on its next mount, which LittleFS can do. What follows was the procedure while the debug table had the smaller file system. - -NVS stays at `0x9000`, so the rolling codes survive. The file system partition shrinks from `0xE0000` to `0xD0000`, and the existing LittleFS won't mount, because `LittleFS.begin()` doesn't format. So: -1. Take a backup through `/backup`. -2. Flash the partition table, firmware and file system over USB: `pio run -e esp32devdbg -t upload -t uploadfs`. -3. Restore the backup. - -## Risks - -- **async_tcp must never wait.** Every handler only queues and returns. The two kinds of waiting reply, the read routes, poll with `RESPONSE_TRY_AGAIN` and never block. -- **Heap.** Edit bodies (one at a time, up to 4 KB) and snapshots (two at a time, about 5.5 KB each) come from the heap. Check the `memStatus` `max` figure on the device after steps 2 and 3. -- **The 2 KB socket event limit** (see KNOWN_ISSUES). That is why `requestResult` carries an id and not the object. -- **The UI changes ship with the firmware** in step 3, because the UI is served from LittleFS. An older UI against the new firmware would wait for HTTP reply bodies that no longer come. - -## Found during the review, not part of this plan - -Nothing is still open. - -Fixed by step 3: -- The Ethernet pin conflict was only checked for the up pin, and the check used the shade's current protocol instead of the one being saved. -- `linkRepeater` never set `isDirty`, so a newly linked repeater wasn't saved. The sort orders weren't saved either. -- `/linkRemote` and `/unlinkRemote` sent a 500 and then a second response. -- The web UI's `procRoomRemoved` kept the removed room and dropped the others, and `deleteShade` never closed its prompt (`prompt.remove;`). -- The web UI called `/repeaters`, which didn't exist. - -Fixed by step 4: -- MQTT `position`/`target` were published as a constant 50, and `direction` as 0 on connect. -- A group whose flags changed sent `groupState` twice. - -Fixed later, by [SHADE_BEHAVIOUR_PLAN.md](SHADE_BEHAVIOUR_PLAN.md) (reconcile Q18): -- `settingPos` was never cleared when the lift arrived, while `settingTiltPos` was, so a later step or group move also sent a stop when it arrived. - -Fixed later, each on its own: -- A restore without shades skipped the shade and group records but not the room records, so it read the rest of the file from the wrong place. The skip is now `shadeModelBytes()` in `ConfigHeader.h`, with native tests (#54). -- New ids stopped two short of the maximum: 30 shades, 14 groups and 14 rooms instead of 32, 16 and 16, an off-by-one from the first commit. They now run to the maximum (#57). -- `SomfyGroup::fromJSON` read `linkedShades` into a local array and dropped it, and wrote past that array when a request listed more than 32 shades. The code is gone; members change only through `/linkToGroup` and `/unlinkFromGroup` (#58). -- The Home Assistant integration sent no `apikey` header on `/shades`, `/groups` and the shade, group and tilt commands, so they failed with security on. It now sends it on every call to a protected route ([ESPSomfy-RTS-HA#4](https://github.com/cjkas/ESPSomfy-RTS-HA/pull/4)). diff --git a/docs/SHADE_BEHAVIOUR_PLAN.md b/docs/SHADE_BEHAVIOUR_PLAN.md deleted file mode 100644 index cb752cd..0000000 --- a/docs/SHADE_BEHAVIOUR_PLAN.md +++ /dev/null @@ -1,169 +0,0 @@ -# Plan: deepen the shade behaviour - -**Status:** done, apart from the device runs. PR A is #46 and PR B is #47 (the behaviour is in `ShadeBehaviour`, pinned by `test_shadebehaviour`). The reconcile fixes Q18–Q22 are one PR each, stacked on #47. Q23 is deferred and Q24 is left for later. -**Written:** 2026-10-16, on top of `refactor/split-transports` (PR #44, the top of the LoopQueue stack #40–#44, which is not yet merged or run on a device). Line numbers refer to that branch. -**Start here:** read [GLOSSARY.md](../GLOSSARY.md), [ADR-0002](adr/0002-loop-task-is-the-only-writer-of-the-shade-model.md) and [LOOP_QUEUE_PLAN.md](LOOP_QUEUE_PLAN.md) first. Their decisions are not reopened here. - -## Why - -This is candidate #1 of the architecture review, the one ranked first. The LoopQueue plan (Q7 there) deliberately did it second, so that the bugs in front of the shade could be fixed first. Steps 1–5 rebuilt everything around the shade. The shade's own behaviour is still about 1,000 lines in [src/SomfyShade.cpp](../src/SomfyShade.cpp). - -### The same rule exists in several drifted copies - -What a shade does for Up, Down or My is written out separately in each place it can be triggered from: - -| Function | Triggered by | -|---|---| -| `processFrame` | a frame received from a remote, and the shade's own echoed frame (see below) | -| `processWaitingFrame` | a remote button held (long press, tilt) | -| `processInternalCommand` | a group command reaching its member shades | -| `sendCommand`, `sendTiltCommand`, `moveToTarget`, `moveToTiltTarget`, `setMyPosition`, `moveToMyPosition` | commands from the web, MQTT and Home Assistant, through ShadeRequests and `SomfyShadeOps` | -| `checkMovement` | every `loop()` pass: simulated travel, arriving at the target, sending the stop, storing the favorite | -| `processTxEvent` | the radio reporting a job started, finished, aborted or dropped | - -Known drift: -- **Up on a tilt-only shade.** `sendCommand` and `processInternalCommand` set target 100, position 100 and tilt target 0. `processFrame` sets only the tilt target. -- **My while a tilt-only shade moves.** `processInternalCommand` sets the lift target to 100; `processFrame` leaves the lift alone. -- **The four "arrived at target" blocks in `checkMovement`** aren't symmetric. The tilt blocks clear `settingTiltPos`; the lift blocks never clear `settingPos`, which may send an extra stop on a later arrival. This is on the "still open" list in LOOP_QUEUE_PLAN.md, not confirmed on a device. Their stop conditions also differ by hand-written literals. -- **`setMyPosition`** has three near-copy branches (tilt-only, tilt, none). - -### None of it is tested - -The native tests can't build it. It reads `millis()`, sends through the radio, and uses the global `somfy`. Only the helpers it calls are tested: `travelPosition`, `stepTargets`, `ShadeSensors`, `TxHold`/`ShadeTxState` and `requestedFavoriteTilt` (`ShadeMotion`, `ShadeSensors`). The fixes just before this work (set-favorite tilt, rolling codes on the main loop) were in this code. - -## What the shade code depends on today - -- **Time:** `millis()` throughout `checkMovement`, `processFrame` and the senders. -- **Radio:** every send goes through `SomfyRemote::sendCommandAfter` ([src/SomfyRemote.cpp](../src/SomfyRemote.cpp), around lines 61–108). It: - 1. takes the next rolling code from NVS; - 2. encodes the frame and submits it with `somfy.sendFrame()`, getting a ticket, or 0 when the radio refuses; - 3. commits the rolling code; - 4. sets `lastFrame`; - 5. calls `somfy.processFrame(lastFrame, internal = true)`; - 6. calls `somfy.markTxPending()` for commands that start movement. -- **The echo (step 5 above).** `somfy.processFrame()` runs `processFrame` on every shade, the sender included. So part of what a command does to the sending shade happens in `sendCommand`, and part in its own `processFrame(internal)` on the echoed frame. The same echo moves any other shade that has the sender's address as a linked remote. GPIO-driven shades (`GP_Remote`/`GP_Relay`) skip the radio but still echo. -- **Groups.** A group command sends the group's frame, then calls `processInternalCommand` on each member shade ([src/SomfyGroup.cpp](../src/SomfyGroup.cpp)). -- **The global `somfy`:** `isDirty`/`commit()` for saving positions and the favorite, `updateGroupFlags()` after sensor and sun-flag changes, `transceiver.config` for bit length and protocol, and `groups` in `isInGroup()`. -- **GPIO:** `triggerGPIOs`, `setGPIOs` and `usesPin` for dry contacts and GPIO motors. -- **Outputs:** only `changed` bits since step 4. The change fan-out publishes them (`ShadeChanges.h`, `SomfyShadeController::sendChanges()`). - -## What makes it easier now - -- **Callers:** ShadeRequests reaches the shade only through the `ShadeOps` seam ([src/ShadeRequests.h](../src/ShadeRequests.h)). The real implementation is `SomfyShadeOps`, and a recording fake exists in `test/test_shaderequests`. Once the behaviour module exists, only `SomfyShadeOps` changes. -- **Outputs:** the shade only marks what changed (step 4). -- **Concurrency:** only `loop()` touches shades (ADR-0002), so the module is single-threaded code. -- **Snapshots:** `shade_snapshot_t` and `snapshotShade()` already describe a shade as clients see it, which is useful for test assertions. - -## The goal - -One module holding the shade's behaviour behind a small interface: -- **Inputs:** a received frame, a command, a group command, a radio event, and a tick, each with the current time. -- **Sending:** goes out through a seam that returns a ticket or a refusal. On the device that is the radio path; in tests, a fake that records what was sent. -- **Inside:** one rule per command and tilt type. -- **Tested** natively, by scenarios such as: *an integrated-tilt shade at 0%; a remote sends Down; at 1.5 s it is still tilting; at 4 s it is lifting at about 25%; reaching the 40% target sends a stop; the radio drops the stop and the shade runs on to 100%.* - -## Design questions - -The decisions are the user's. Each question comes with a recommended answer, and they are asked in rounds, as in the LoopQueue plan. Settled answers move to "Decisions". - -### Found while preparing round 2 - -- **The echo does part of the sender's work, and it varies by command.** For a frame marked internal, `processFrame` leaves the targets alone on Up and Down (the sender sets them itself after the send). It does set them on My (when idle), Stop, Toggle, Favorite, StepUp and StepDown. So for Stop, Step, Toggle and Favorite sent from the web, the shade's *only* reaction is in its echo. A linked shade gets the same internal frame, so it follows My/Stop/Step/Toggle/Favorite but not Up/Down. The echo also resets `moveStart`/`startPos` and copies the frame into `lastFrame` on every shade it reaches. -- **The order inside a send is:** encode → submit → commit rolling code → `lastFrame` → echo to every shade → `markTxPending` on the sender, its linked shades and the members of a group with that address. The sender's caller (`moveToTarget`, `sendCommand`, …) sets its targets *after* all of that. -- **`lastFrame` serves two purposes.** It is the template `sendCommandAfter` builds the next frame from, and the long-press state `processWaitingFrame` reads (`await`, `repeats`, `processed`, `cmd`, `stepSize`). -- **`emitCommand` is still a direct output.** It builds a socket event with ArduinoJson and publishes three MQTT topics on the spot, about 40 times in the shade code and once per member in `SomfyGroup::sendCommand`. `emitState()` only marks the shade changed. -- **`isInGroup()` isn't used by the behaviour code.** Only the delete check in SomfyJson and the snapshot call it, so it stays in `SomfyShade`. -- **`flags` mixes settings and state.** SimMy, SunSensor and Light are settings; SunFlag, Sunny, Windy and DemoMode are sensor state the behaviour changes. ConfigFile saves the whole byte. -- **Outside writers of motion state:** ConfigFile at boot (positions, targets, favorite) and `SomfyShadeOps` setting a position without moving (`p_target`/`p_currentPos`). The other files only read it. - -### Found while preparing round 3 - -- **The sender's own echo needs the frame as sent, not just the command.** The Flag and SunFlag rules ignore a frame whose rolling code is 0x8000 or above (a guard against bogus sensors), and they apply it to the shade's own echo too. `flipCommands` flips the command on the way out and flips it back when the echo comes in. -- **`sendSensorCommand` doesn't move.** ShadeRequests calls it through `SomfyShadeOps`, and it stays in `SomfyRemote`. It keeps echoing to every shade, the sender included, so the shade hears it through `onFrame`. -- **GPIO:** `GP_Remote` pulses its pins inside the send; `GP_Relay` sends nothing and `setGPIOs()` drives the relays from `loop()` by reading the direction. Both stay outside the module. - -### Still open - -- **Reconcile.** Every candidate is now fixed, deferred or left for later (Q17–Q24). Each was pinned by a "today:" test in `test_shadebehaviour` unless marked: - - ~~`settingPos` is never cleared on a lift arrival, so a later step also sends a stop.~~ Fixed (Q18). - - ~~Up and My on tilt-only shades differ between the web, a remote and a group.~~ Fixed (Q22). - - An integrated tilt sent to a lift target without a tilt turns its slats back with an Up after the stop (`test_today_integrated_tilt_turns_back_after_a_lift_target`). Deferred (Q23). - - ~~A remote step uses the step size this shade last *sent*.~~ Fixed (Q19). - - ~~A group's SunFlag or Flag doesn't make other groups work their flags out again.~~ Fixed (Q20). - - The self-echo (Q5) and merging group commands into the frame rule (Q8). Possible later (Q24). - - `lastFrame` doing two jobs (the Q6 revisit). Possible later (Q24). - - ~~The 500 ms long-press deadline isn't wrap-safe.~~ Fixed (Q21). - -## Constraints - -- **[AGENTS.md](../AGENTS.md):** every change keeps `pio test -e native` green and adds or updates tests. -- **Flash:** check every commit. `esp32dev` (release, with OTA) has about 180 KB free. The debug build uses one 3 MB slot once #39 is in its history. Every file that uses ArduinoJson costs flash, so keep the new module free of it. -- **`ShadeMotion`, `ShadeSensors`, `ShadeTxState` and `stepTargets`** are deep and tested. Reuse them; don't re-derive them. -- **Vocabulary:** module, interface, seam, adapter, depth (the `/codebase-design` skill) and the domain terms in GLOSSARY.md. - -## Decisions - -**Round 1 (2026-10-16), all as recommended:** - -- **Q1. What moves in.** `processFrame`, `processWaitingFrame`, `processInternalCommand`, `sendCommand`, `sendTiltCommand`, `moveToTarget`, `moveToTiltTarget`, `setMyPosition`, `moveToMyPosition`, `checkMovement` and `processTxEvent` move into the module. So do their helpers (`sendStop`, `sendSetMy`, `storeMyPosition`, `runToLimit`, `applySensorMove`, `applyStep`, `tiltRepeats`) and their state: positions, targets, directions, `lastMovement`, start times and positions, the `setting*` flags, `awaitMy`, the favorite, `ShadeSensors` and `ShadeTxState`. The `p_*` setters and change marking also move in. `SomfyShade` keeps identity, settings, persistence, GPIO, JSON, MQTT topics and rolling codes. -- **Q2. Pin, move, reconcile.** Phases 1 and 2 land together and change no behaviour. Disagreements are pinned as "today: …" tests. Reconcile makes one decision per commit, each checked on real motors. -- **Q3. Time is passed in.** Every input carries `now` as `uint32_t`, with wrap-safe elapsed times as in `TxHold`. -- **Q4. One object per shade.** It holds a `const ShadeSettings&`. The settings fields move into that struct, which `SomfyShade` owns, so there is one copy and nothing to drift. - -**Round 2 (2026-10-16), all as recommended.** The principle: the module never touches anything outside itself; `SomfyShade` wraps it and does the outside work. - -- **Q5. The echo.** Today a sent frame is handed back to every shade as if a remote had sent it, and some commands (Stop, Step, Toggle, Favorite, My when idle) do their work only there. The module calls its own frame rule directly after a successful send, at the same point as today. Other shades (linked remotes) still hear the frame through the controller. Whether the sender should hear itself at all is a reconcile decision. -- **Q6. Held-button state.** The module keeps its own record of the held button (`cmd`, `repeats`, `await`, `processed`, `stepSize`, `remoteAddress`). `SomfyRemote::lastFrame` stays the send template, and `SomfyShade` keeps copying frames into it as today. -- **Q7. Command events.** They leave through an output seam, `onCommand(cmd, source, sourceAddress)`, which calls today's `emitCommand` on the device and records in tests. The timing is unchanged, and ArduinoJson stays out of the module. -- **Q8. Group commands.** They stay a separate input, `onGroupCommand(cmd, repeat, now)`, with today's rule. `markTxPending` becomes `onTxQueued(ticket, now)`. Merging group commands into the frame rule is a reconcile decision. -- **Q9. The global `somfy`.** The module reports "save wanted" and "sensor flags changed"; `SomfyShade` sets `somfy.isDirty` and calls `updateGroupFlags()`. `bitLength` goes into `ShadeSettings`, still filled in lazily by `SomfyShade`. `isInGroup()` stays in `SomfyShade`. -- **Q10. The `flags` byte.** SunFlag, Sunny, Windy and DemoMode go into the module; SimMy and SunSensor into `ShadeSettings`; Light stays in `SomfyShade`. The byte is put back together for ConfigFile, JSON and MQTT, so nothing saved or sent changes. -- **Q11. Reading the motion state.** A read-only `state()`. ConfigFile at boot writes through `restore(...)` and `SomfyShadeOps` through `setPosition(pos, tilt)`. The 12 reading files switch over in the move phase. - -**Round 3 (2026-10-16), all as recommended:** - -- **Q12. The radio seam.** The module sends with `send(cmd, repeats, stepSize, delayMs, priority)` and gets back `{sent, ticket, frame}`. On the device, `SomfyRemote` does what `sendCommandAfter` does today (rolling code, encode, submit or pulse GPIO, commit the code, `lastFrame`), but it echoes and marks the radio hold only for the *other* shades and group members. The module runs its own echo on the returned frame and marks its own hold. In tests a fake records each send, hands out tickets 1, 2, 3, … and can be told to refuse. -- **Q13. Names.** These go into GLOSSARY.md with PR B, when the code exists: - - **ShadeBehaviour**: the module (`src/ShadeBehaviour.h/.cpp`, `test/test_shadebehaviour`). - - **ShadeSettings**: the read-only settings. - - **ShadeBehaviour::State**: the motion state `state()` returns. - - **MotorLink**: the send seam, radio or GPIO. - - **ShadeEvents**: the `onCommand(...)` output. -- **Q14. Order and PRs**, stacked on #44: - 1. **PR A:** `ShadeSettings` and the `flags` split, with tests that the saved byte and JSON don't change. - 2. **PR B:** the move: `ShadeBehaviour`, `MotorLink`, `ShadeEvents`, `restore`/`setPosition`, the 12 readers and the pinning scenario tests. No behaviour change. - 3. **Reconcile:** one PR per decision. These start only after #40–#44, A and B have run on a device. - -**Before PR A (2026-10-16), all as recommended.** Q4 assumed every setting lives on `SomfyShade`. But `flipCommands`, `bitLength`, `repeats` and the `flags` byte are on the `SomfyRemote` base, which groups share and the send path uses. - -- **Q15. Two settings structs, held as bases.** `RemoteSettings` (`flipCommands`, `bitLength`, `repeats`) is a base of `SomfyRemote`, so shades and groups each have one. `ShadeSettings` (types, travel times, step size, `flipPosition`, `simMy`, `sunSensor`) is a second base of `SomfyShade`. Existing code like `shade->upTime` keeps compiling. The module will get two read-only references. That is still one copy of each, so nothing can drift. -- **Q16. The `flags` byte.** - - SimMy and SunSensor are `bool`s in `ShadeSettings`, and Light is a `bool` on `SomfyShade`. - - SunFlag, Sunny, Windy, DemoMode and the unused Lighted stay on `SomfyShade` as `stateFlags` until PR B moves them into the module. - - A shade's byte is `getFlags()` (put together) and `setFlags()` (split), used by ConfigFile, JSON, MQTT, events and group flags. - - Groups keep their own `flags` byte (the OR of their shades), and `SomfyRemote::getFlags()` is virtual for `sendSensorCommand`. - - `test_shadesettings` checks that all 256 byte values come back unchanged. - -**During PR B (2026-10-16):** - -- **Q6 revisited: `lastFrame` is shared.** The module holds a reference to `SomfyRemote::lastFrame` instead of its own copy. A copy would have to mirror five writes exactly: each send overwrites it; the shade's own echo then counts as a repeat; `repeatFrame` adds the queued repeats; a sensor send overwrites it; and `copy()` never copies `stepSize` or `await`. The user chose sharing. Splitting the two jobs is a reconcile candidate. -- **Q12 refined.** `MotorLink::send` returns `{sent, ticket}`; the frame as sent is in the shared `lastFrame`. `SomfyRemote::sendCommandAfter(…, hearsItself = true)` leaves the sender out of the echo and the radio hold; `ShadeBehaviour::transmit` does both for itself. -- **Q7 and Q9 refined: one output seam that fires on the spot.** `ShadeEvents` carries `onChanged` (the change bits), `onCommand`, `onSaveWanted` and `onSensorFlags`. Nothing is collected and read back after each call, so the timing is today's. `onSensorFlags` fires exactly where `somfy.updateGroupFlags()` was called, not on every sensor-bit change. -- **`onTxEvent` takes no `now`:** the event carries its own time. -- **A known ordering change:** when a simMy shade's Favorite or My sends again from inside its own echo, a shade linked to it now hears the outer frame before the inner one. Before, it depended on the two shades' slots. Nothing else changes order. -- **Flash:** `esp32dev` `firmware.bin` grows 1,397,776 → 1,399,776 B (+2,000 B, 88.6%), mostly the virtual calls through the two seams. - -**Reconcile round 1 (2026-10-16), all as recommended.** Each fix is one PR stacked on #47, and each changes a "today:" test. - -- **Q17. Order.** Clear bugs first: `settingPos` (Q18), the remote step size (Q19), group sun flags (Q20), the long-press wrap (Q21). Then the rules where the copies disagree: tilt-only Up/Down/My, and the integrated tilt turning back. The structural changes (self-echo, merging group commands, splitting `lastFrame`) wait until those are done and may be dropped. -- **Q18. `settingPos` is cleared on a lift arrival,** as `settingTiltPos` is on a tilt arrival. A later step or group move then stops by itself instead of getting a My, which would have sent a stopped motor to its favorite. -- **Q19. A remote's step uses the frame's own step size** (`frame.stepSize`, 1 when 0) instead of `lastFrame.stepSize`, which `copy()` never updates. `copy()` itself stays as it is. -- **Q20. A group's SunFlag/Flag calls `onSensorFlags()`,** as the frame rule does, so every group works its flags out again. -- **Q21. The long-press wait is wrap-safe:** `(int32_t)(now - await) > 0`. - -**Reconcile round 2 (2026-10-16):** - -- **Q22. A tilt-only shade's lift is pinned at 100 on every path** (web, remote, group; Up, Down, My, idle or moving), and those commands act on the tilt only. Before, the remote and group My paths and the web's My while moving left a lift that something had moved, and `tick` then simulated it travelling. `test_tilt_only_lift_stays_down_whatever_the_source` checks the 18 combinations. -- **Q23. Integrated tilt turning back: deferred.** No integrated-tilt motor is at hand to watch, and reconcile only changes what has been checked on a real motor. The behaviour stays, still labelled "today:", until someone with such a shade can say whether the turn-back is clean. -- **Q24. The structural changes are left for later:** dropping the self-echo, merging group commands into the frame rule, and splitting `lastFrame`'s two jobs. None of them fixes anything a user would notice, each touches every command path, and the behaviour they would move is now pinned by tests, so they can be picked up any time. -- **Q25. One PR per fix,** stacked on #47, each run on a device before it merges. diff --git a/docs/SOMFY_SPLIT_PLAN.md b/docs/SOMFY_SPLIT_PLAN.md deleted file mode 100644 index 5b61edd..0000000 --- a/docs/SOMFY_SPLIT_PLAN.md +++ /dev/null @@ -1,116 +0,0 @@ -# Plan: split and refactor Somfy.cpp - -**Status:** done. Phases 0 and 1 in `bbfb23e`..`505108c`, phase 2 in `0eb0e63`..`a176017` (2026-10-13). -**Written:** 2026-10-13, against commit `4544eb6` on `fix/live-bugs`. Line numbers in the analysis below refer to that commit. The code now lives in the files listed under [Phase 1](#phase-1-split-the-file-by-concern). -Since then `SomfyMqtt.cpp` and `SomfyEvents.cpp` have moved to `src/outputs/`. - -## Why - -`src/Somfy.cpp` is 4,766 lines. About 690 of them never compile. Six classes, two config structs and the receive interrupt handler share the file. The methods of each class are scattered: `SomfyShade`'s 71 methods are spread over about 20 separate runs. Some logic exists in several copies that have already drifted apart (see [Problems](#problems-beyond-file-size)). - -## What the file contains - -| What | Lines | Live lines (approx.) | Changes when… | -|---|---|---|---| -| Dead code: `/* */` blocks and `#ifdef USE_NVS` blocks (`USE_NVS` is never defined) | everywhere | ~690 (392 + 296) | never | -| Radio: `Transceiver`, `transceiver_config_t`, receive interrupt and pulse decoder, radio task, frequency scan, `RadioLock`/`TxQueueLock` | 1–116, 3700–4766 | ~950 | the radio hardware or RX/TX changes | -| MQTT: `publish*`/`unpublish*` for shades, groups and rooms, plus Home Assistant discovery | 980–1348, 3113 | ~400 | the MQTT topics change | -| Socket events: `emitState`/`emitCommand` | 1451–1611, 3105 | ~90 | the socket protocol changes | -| JSON config: `validateJSON`/`fromJSON` | 2775–3066 | ~200 | the web config form changes | -| GPIO and dry contacts: `setGPIOs`, `triggerGPIOs`, `usesPin` | 570–695, 2758 | ~140 | the relay wiring changes | -| `SomfyRemote`: rolling codes, `sendCommandAfter`, `repeatFrame` | scattered | ~200 | how frames are sent changes | -| Shade behaviour: `processFrame`, `processInternalCommand`, `processWaitingFrame`, `checkMovement`, my-position, `moveTo*`, `sendCommand` | 696–923, 1612–2706 | ~1,350 | what a shade does | -| `SomfyShadeController` and `SomfyRoom`: add/delete/find, ids, commit | 117–300, 3076–3700 | ~600 | the shade list changes | - -## Problems beyond file size - -1. **The same logic exists in several copies, and the copies have drifted apart.** - - **Step targets.** `processFrame` uses the tested `stepPosition()` from `ShadeMotion`. `processInternalCommand` (~2172–2242) has its own inline maths, which ignores the frame's step count. `processWaitingFrame` uses a fixed 1% step. - - **Sun/wind.** Sensor state is set in the `Sensor` case of `processFrame`, acted on by timers in `checkMovement`, and checked again in the `Down` case of `processInternalCommand` (`windLast`). The SunFlag handling exists twice. **These look like real bugs for tilt-only shades:** - - the copy in `processFrame` (~1865) moves the tilt target of a tilt-only shade; - - the copy in `processInternalCommand` (~2262) moves the lift target instead; - - the sun timer in `checkMovement` (~744) also moves the lift target of a tilt-only shade, while the no-sun timer two branches later moves the tilt. -2. **The receive pulse decoder has no tests.** `handleReceive` (~3839–3969) turns pulse durations into bits. It is pure logic, so recorded pulse trains could test it in the native env, the way `SomfyFrame` is tested. -3. **`SomfyShade` has a wide interface:** 6 `publish` overloads, 12 `p_*` setters, `public` fields, and seven sun/wind fields (timestamps and done flags) that are really one sensor state. -4. **Hidden dependencies.** Shade methods use the global `somfy` for `isDirty`, `commit()`, `transceiver.config` and `updateGroupFlags()`. This plan leaves them alone. Removing them belongs to the "only `loop()` owns the shade model" work in [KNOWN_ISSUES.md](KNOWN_ISSUES.md). - -## Plan - -### Phase 0: delete the dead code -**Done** in `bbfb23e`. `sort_asc()` was dead too (only called from a dead block) and went with it. - -One commit, no behaviour change. - -- Remove every `#ifdef USE_NVS` block (296 lines), including `loadLegacy()` and `SomfyShade::load()`. Also remove `useNVS()`: it sits outside the blocks, but only the blocks call it. -- Remove the 16 commented-out blocks (392 lines), mostly old `toJSON` versions and `snprintf` emitters. -- Remove the matching declarations in `Somfy.h`. - -This takes the file to about 4,050 lines before anything moves. - -### Phase 1: split the file by concern -**Done** in `0a329ca`..`505108c`, one commit per file. Code moved unchanged, apart from `bit_length`, which became the shared `radioBitLength`. The 253 top-level definitions before and after match one for one. - -Several commits. Code moves as it is: the classes and `Somfy.h`'s interface stay the same, only the files change. A class's methods may end up in several files by concern. Check each move with the `esp32devdbg` build and the native tests. - -| New file | Lines | Contents | -|---|---|---| -| `Transceiver.h/.cpp` | 125 + 949 | `Transceiver`, `transceiver_config_t`, `RadioLock`/`TxQueueLock` and the radio statics, the receive interrupt handler and RX queue, the radio task, the frequency scan, and `somfy_frame_t::decodeFrame(somfy_rx_t*)` (it needs `RadioLock`). `Somfy.h` includes `Transceiver.h`, so callers don't change. | -| `SomfyRemote.cpp` | 170 | `SomfyRemote` and `SomfyLinkedRemote` | -| `SomfyShade.cpp` | 1,532 | shade behaviour (the rest of `Somfy.cpp`, renamed so history follows it) | -| `ShadeGpio.cpp` | 148 | the GPIO and dry-contact methods | -| `SomfyGroup.cpp` | 134 | `SomfyGroup` behaviour | -| `ShadeController.cpp` | 449 | `SomfyShadeController` and `SomfyRoom`. Not `SomfyController.cpp`, which would look like a pair with `SomfyController.ino`. | -| `SomfyMqtt.cpp` | 412 | every `publish*`/`unpublish*`/`publishDisco` | -| `SomfyEvents.cpp` | 99 | every socket `emitState`/`emitCommand` | -| `SomfyJson.cpp` | 235 | `validateJSON`/`fromJSON` for shades, groups and rooms, **and the radio's** (see below) | - -#### Flash size: what the split taught us - -The `esp32devdbg` app partition is 1,572,864 bytes, and before the split the image used 99.4% of it. - -- At `-Og`, GCC only caps inlining growth in units over `large-unit-insns` (10,000 instructions). The big `Somfy.cpp` was over that, so ArduinoJson's helpers stayed calls. In smaller files they were inlined into every caller: moving the JSON parsing out grew the image by 19 KB, past the limit. `--param=large-unit-insns=1000` in `[env:esp32devdbg]` (`4cb676a`) treats every file as large. That freed 13 KB, and the split itself then cost nothing. -- Every file that uses ArduinoJson also gets its own copy of ArduinoJson's debug-assert strings, and its own copies of helpers it inlines. So all JSON parsing lives in `SomfyJson.cpp`, including the radio's `fromJSON`, which the plan had kept in `Transceiver.cpp`. Grouping it saved 2.8 KB. `SomfyMqtt.cpp` (`publishDisco` builds a `JsonDocument`) is the only other new file that uses ArduinoJson. -- Result: 1,546,797 bytes (98.3%), 16.8 KB less than before phase 0. - -For phase 2, keep new modules free of ArduinoJson, and check the flash size of every commit. If space runs out, remove log lines first. - -### Phase 2: pull out the copied logic -**Done** in `0eb0e63`..`a176017`. Each module is plain C++ like `ShadeMotion`, with native tests (222 tests in all now): - -| Module | What it holds | -|---|---| -| `ShadeTypes.h` | `group_types`, `shade_types` and `tilt_types`, moved out of `Somfy.h` so the modules below can use them in the native tests. | -| `ShadeSensors.h/.cpp` | The sun and wind timers and their rules: `onSensorFrame()`, `check()`, `onSunFlag()`, `windBlocksDown()`. `sensorTargets()` turns extend/retract into targets for all three callers. | -| `stepTargets()` in `ShadeMotion` | Where a step takes a shade, shared by received steps (`processFrame`) and group steps (`processInternalCommand`). | -| `PulseDecoder.h/.cpp` | `decodePulse()`, the RTS timings and `somfy_rx_t`, moved out of `handleReceive`. It stays in IRAM (`0x4008xxxx`), and the tests feed it the edges `sendFrame()` produces. | - -What changed in behaviour: -- **Tilt-only shades and the weather** (`5f26340`): sun and wind now move only the tilt of a tilt-only shade, to the favorite tilt (or fully) and back to 0, and only the lift of every other shade. Before, each of the three callers did something different. -- **Sun and wind timers across the `millis()` rollover**: the timers are now 32-bit like `millis()`. As 64-bit values they fired at once after the rollover every 49.7 days. This also saves 768 bytes of RAM. -- **Step edge cases**: a group step on a tilt-only shade already at its end now goes through like a remote step. A step that can't be sized (no travel time) leaves the targets alone. -- The 8 debug logs of the sun and wind timers are gone. They were compiled out at `CORE_DEBUG_LEVEL=3`. - -Kept as it was: the fixed 1% step of a held step button in `processWaitingFrame`. It later turned out never to run: a step frame is acted on as soon as it arrives, so it is never left waiting. The branch was deleted, and `test_a_held_step_button_steps_once` pins what a held step button does. - -Left out on purpose, since neither fixes a bug or can be reached by the native tests: -- the four near-copy arrive-at-target blocks in `checkMovement`. They are now in `ShadeBehaviour::tick` ([SHADE_BEHAVIOUR_PLAN.md](SHADE_BEHAVIOUR_PLAN.md)), still four copies, but pinned by native tests; -- the six `getNext*Id`/`getMax*Order` copies in the controller. Since replaced by ModelEditor's `firstFreeId` (step 3 of [LOOP_QUEUE_PLAN.md](LOOP_QUEUE_PLAN.md)). - -Flash went from 1,546,797 to 1,545,717 bytes. - -## Risks - -- **IRAM.** The decoder runs inside the interrupt and must stay in IRAM. Watch the build for `dangerous relocation` or IRAM link errors. -- **Flash.** See [Flash size](#flash-size-what-the-split-taught-us). -- **Inlining flag.** `--param=large-unit-insns=1000` changes inlining across the whole debug build. Check on the device that radio RX/TX still work. - -## Decisions - -Decided for phases 0 and 1: -- Phases ran in the order 0 → 1. -- Headers: `Somfy.h` stays the one header, with only `Transceiver.h` split out. -- Log tags: every file keeps `"Somfy"`. Files that never log (`SomfyMqtt.cpp`, `SomfyEvents.cpp`) have no tag. - -Decided for phase 2: -- The tilt-only sun/wind bugs were fixed in their own commit, right after `ShadeSensors` was extracted with behaviour unchanged. -- A tilt-only shade's lift is never moved by the weather. diff --git a/docs/adr/0002-loop-task-is-the-only-writer-of-the-shade-model.md b/docs/adr/0002-loop-task-is-the-only-writer-of-the-shade-model.md index 9fdea0a..8b01356 100644 --- a/docs/adr/0002-loop-task-is-the-only-writer-of-the-shade-model.md +++ b/docs/adr/0002-loop-task-is-the-only-writer-of-the-shade-model.md @@ -15,5 +15,9 @@ The shade model (rooms, shades, groups and repeaters) is changed only from `loop - async_tcp must still never wait for `loop()`, so a model edit's HTTP reply can't block until the change is made. The reply only says the request was accepted; the result reaches clients as a WebSocket event once `loop()` has made the change. We chose that over a chunked HTTP response that `loop()` completes, because pushing results to clients is what the WebSocket is for and it needs no pool of pending responses. - A read has to reply with data, so it does wait, without blocking: its chunked response sends nothing until `loop()` has filled the read's ModelSnapshot. -- MQTT commands already run on `loop()` (`mqtt.loop()` is called from it). +- MQTT commands already run on `loop()` (`mqtt.loop()` is called from it), but they go through the LoopQueue too. That gives them the radio back-pressure web commands have (`holdWebCommand`): while the radio is full they wait instead of being refused, and they run in the order they arrived with web requests. +- The LoopQueue is one FIFO, and no item jumps it, model edits included. +- Requests that hold heap are capped, so a burst can't exhaust it: at most one model edit (a copy of its body, up to 4 KB; a larger one gets 413) and two reads (a ModelSnapshot of about 5.6 KB each) at a time. One more gets 503 "Busy", and the web UI retries it. +- The web UI waits about 5 s for an edit's `requestResult`. If none comes, it says the change wasn't confirmed and offers to reload. +- The UI and the firmware change together, because the UI is served from LittleFS. An older UI against firmware with this rule waits for edit reply bodies that no longer come. - With one writer, the domain modules are plain single-threaded code and can be tested natively without threads. diff --git a/src/SomfyJson.cpp b/src/SomfyJson.cpp index af6a53f..a79dcf7 100644 --- a/src/SomfyJson.cpp +++ b/src/SomfyJson.cpp @@ -214,7 +214,8 @@ void transceiver_config_t::fromJSON(JsonObject& obj) { /********************************************************************* * ModelEditor on the device (SomfyEditor.h). Here rather than in a file of * its own because every file that uses ArduinoJson carries its own copy of the - * library's helpers (docs/SOMFY_SPLIT_PLAN.md). + * library's helpers and assert strings (moving the radio's fromJSON here saved + * 2.8 KB of flash). ********************************************************************/ static std::atomic editBusy(false); static std::atomic lastRequestId(0); diff --git a/test/test_shadebehaviour/test_shadebehaviour.cpp b/test/test_shadebehaviour/test_shadebehaviour.cpp index 0f63b11..eed240b 100644 --- a/test/test_shadebehaviour/test_shadebehaviour.cpp +++ b/test/test_shadebehaviour/test_shadebehaviour.cpp @@ -4,7 +4,7 @@ // Scenario tests pinning what ShadeBehaviour does today, moved unchanged from // SomfyShade. Where the copies of a rule disagree the test says "today:" so -// a later reconcile shows up as a test change (SHADE_BEHAVIOUR_PLAN.md). +// a later reconcile shows up as a test change (the one left is #85). #define ASSERT_POS(expected, actual) TEST_ASSERT_FLOAT_WITHIN(0.5f, (expected), (actual)) #define ASSERT_CMD(expected, actual) TEST_ASSERT_EQUAL_HEX8(static_cast(expected), static_cast(actual))