feat(daplink_flash): Add file-IO driver on top of the DaplinkBridge.#167
feat(daplink_flash): Add file-IO driver on top of the DaplinkBridge.#167nedseb wants to merge 22 commits into
Conversation
nedseb
force-pushed
the
feat/daplink-flash-driver
branch
from
4b56a90 to
5570c49
Compare
|
Aline, Ce qui se passe pendant le rebaseJ'ai droppé tes 5 commits sur
C'est sur quoi cette revue porte. J'ai déjà adapté le code pour qu'il compile contre mainToutes tes méthodes Plutôt que d'attendre que tu te débloques toute seule, j'ai poussé deux commits sur la branche :
65/65 tests natifs passent maintenant, mais j'ai dû désactiver 2 tests avec un TODO explicite ( À toi de prendre la suite à partir de là — il reste les conventions, le design API et le rework des tests. Conventions à appliquer (familier après #143 / #152 / #161)Les mêmes points que sur tes PR précédentes. Liste à cocher au passage :
Design API à challengerQuelques points où le shape de l'API se discute :
Tests — les mêmes patterns à corriger qu'avantTu as la même structure de test que sur le bridge avant qu'on la corrige. Notamment :
Le fix #161 te montre le pattern propre — une fois que tu fixes les autres points, les tests pourront utiliser Manques pour que ce soit complet
Plan
Pas de rush — la branche est rebasée, ton code compile, tu peux travailler dessus sans pression de conflit. Et pour les conventions, regarde aussi les retours pédagogiques sur #143, #152, #161 ; les patterns reviennent. |
There was a problem hiding this comment.
Pull request overview
Adds a new daplink_flash driver intended to sit on top of DaplinkBridge to perform flash/file-like operations (8.3 filename handling, raw writes, sector reads) over the DAPLink F103 I2C bridge, along with native tests and documentation.
Changes:
- Introduces
DaplinkFlash(header/impl + protocol constants) and adds a README + Arduino library metadata. - Extends
DaplinkBridgewith frame-level primitives (sendCommand,readResponse) and aREG_RESPONSEconstant for sibling drivers. - Adds a native unit test suite for
daplink_flashand wires the driver into the repo’s smoke-test sketch.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/native/test_daplink_flash/test_main.cpp | Adds host-side Unity tests for the new flash driver. |
| src/main.cpp | Instantiates DaplinkFlash in the build smoke-test sketch and probes begin(). |
| lib/daplink_flash/src/daplink_flash.h | Declares the DaplinkFlash public API. |
| lib/daplink_flash/src/daplink_flash.cpp | Implements filename management, write helpers, and sector-based reads via the bridge. |
| lib/daplink_flash/src/daplink_flash_const.h | Defines flash-level command IDs and protocol limits. |
| lib/daplink_flash/README.md | Documents hardware context, quick start, and the public API. |
| lib/daplink_flash/library.properties | Registers the Arduino library and declares dependency on the bridge library. |
| lib/daplink_bridge/src/DaplinkBridge.h | Exposes new framing helpers for sibling drivers. |
| lib/daplink_bridge/src/DaplinkBridge.cpp | Implements sendCommand and readResponse. |
| lib/daplink_bridge/src/daplink_bridge_const.h | Adds DAPLINK_BRIDGE_REG_RESPONSE constant. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Wire.clearWrites(); | ||
| flash.clearFlash(); | ||
|
|
||
| bool sawCmd = false; | ||
| for (const auto& w : Wire.getWrites()) { | ||
| if (w.reg == DAPLINK_FLASH_CMD_CLEAR_FLASH) { | ||
| sawCmd = true; | ||
| } | ||
| } | ||
|
|
||
| TEST_ASSERT_TRUE(true); |
|
Good work, I only have a few change requests before merge:
Once those are cleaned up, I’m happy with the direction of the PR. |
|
Quick follow-up after the failed integration-test run on real hardware: |
| readBlock(static_cast<uint8_t>(DAPLINK_BRIDGE_REG_RESPONSE + produced), buf + produced, | ||
| want); |
There was a problem hiding this comment.
fix in 8b3b27f — readResponse now emits the cmd byte once and streams the response through successive requestFrom() calls without ever recomputing REG_RESPONSE+produced, so the 0x82+0x7D=0xFF wraparound disappears. The chunk loop also no longer calls waitNotBusy()/readBlock() between chunks, which were the two things that reset the firmware response cursor.
| uint8_t endTransmission(bool = true) { | ||
| if (txBuffer_.size() >= 2) { | ||
| // [reg, val0, val1, ...] — I2C auto-increment: each value lands at | ||
| // reg + offset, one WriteOp per byte so tests can assert the full | ||
| // write sequence. | ||
| uint8_t reg = txBuffer_[0]; | ||
| for (size_t i = 1; i < txBuffer_.size(); ++i) { | ||
| uint8_t targetReg = static_cast<uint8_t>(reg + (i - 1)); | ||
| uint8_t val = txBuffer_[i]; | ||
| registers_[makeKey(currentAddress_, targetReg)] = val; | ||
| writes_.push_back({currentAddress_, targetReg, val}); | ||
| } | ||
| currentRegisterByAddr_[currentAddress_] = reg; | ||
| } else if (txBuffer_.size() == 1) { | ||
| currentRegisterByAddr_[currentAddress_] = txBuffer_[0]; | ||
| uint8_t cmd = txBuffer_[0]; | ||
| commands_.push_back({currentAddress_, cmd}); | ||
| currentRegisterByAddr_[currentAddress_] = cmd; | ||
| } |
There was a problem hiding this comment.
fix in 9c6cc64 — requestFrom() now pops the trailing entry from commands_ when it matches the current address, so a register-pointer-select single-byte transmission followed by a read no longer pollutes getCommands() with a phantom command.
| const auto& writes = Wire.getWrites(); | ||
| if (writes.size() >= 10) { | ||
| payloadCorrect = | ||
| (writes[0].value == 'T') && (writes[1].value == 'E') && (writes[2].value == 'S') && | ||
| (writes[3].value == 'T') && (writes[4].value == ' ') && (writes[5].value == ' ') && | ||
| (writes[6].value == ' ') && (writes[7].value == ' ') && (writes[8].value == 'T') && | ||
| (writes[9].value == 'X') && (writes[10].value == 'T'); | ||
| } |
There was a problem hiding this comment.
fix in 9c6cc64 — bumped the size check from >= 10 to >= 11 so the writes[10] read is bounds-safe.
| void setUp(void) { | ||
| // begin() on the flash driver re-probes the bridge, so we can call | ||
| // it before every test without any extra plumbing. | ||
| flash.begin(); | ||
| } | ||
|
|
||
| void tearDown(void) {} | ||
|
|
||
| void test_daplink_flash_begin() { | ||
| TEST_ASSERT_TRUE_MESSAGE(flash.begin(), "flash.begin() must succeed on a wired STeaMi"); | ||
| } | ||
|
|
||
| void test_daplink_flash_get_filename_returns_printable_chars() { | ||
| // Read the current filename. Whatever it contains, every byte that | ||
| // survives the trailing-space trim should be a printable ASCII | ||
| // character — non-printable garbage would mean the read framing or | ||
| // the readResponse byte count is broken. | ||
| auto current = flash.getFilename(); | ||
|
|
||
| for (size_t i = 0; i < strlen(current.name); ++i) { | ||
| unsigned char c = static_cast<unsigned char>(current.name[i]); | ||
| TEST_ASSERT_TRUE_MESSAGE(c >= 0x20 && c < 0x7F, "filename contains non-printable byte"); | ||
| } | ||
| for (size_t i = 0; i < strlen(current.ext); ++i) { | ||
| unsigned char c = static_cast<unsigned char>(current.ext[i]); | ||
| TEST_ASSERT_TRUE_MESSAGE(c >= 0x20 && c < 0x7F, "extension contains non-printable byte"); | ||
| } | ||
| } |
There was a problem hiding this comment.
fix in 9c6cc64 — setUp() now asserts flash.begin() so a missing bridge fails fast instead of letting empty filename strings make the printable-ASCII test pass vacuously.
| category=Data Storage | ||
| url=https://github.com/steamicc/arduino-steami-lib | ||
| architectures=* | ||
| includes=daplink_flash.h |
There was a problem hiding this comment.
Acknowledged — deferring to a follow-up PR/issue to keep this one focused on the review feedback. Tracking the rename of daplink_flash.{h,cpp,_const.h} (and the matching library.properties / README / #include sites) separately so it can be reviewed on its own.
| void DaplinkFlash::setFilename(const char* name, const char* ext) { | ||
| // Set 8.3 filename. name: max 8 chars, ext: max 3 chars. | ||
| if (name == nullptr || ext == nullptr) { | ||
| return; | ||
| } | ||
| char n[DAPLINK_FLASH_FILENAME_LEN]; | ||
| size_t nameLen = std::min(strlen(name), (size_t(DAPLINK_FLASH_FILENAME_LEN))); | ||
| for (int i = 0; i < nameLen; i++) { | ||
| n[i] = toupper((unsigned char)name[i]); | ||
| } | ||
|
|
||
| char e[DAPLINK_FLASH_EXT_LEN]; | ||
| size_t extLen = std::min(strlen(ext), (size_t(DAPLINK_FLASH_EXT_LEN))); | ||
| for (int j = 0; j < extLen; j++) { | ||
| e[j] = toupper((unsigned char)ext[j]); | ||
| } | ||
|
|
||
| uint8_t padded[DAPLINK_FLASH_FILENAME_LEN + DAPLINK_FLASH_EXT_LEN]; | ||
| memset(padded, ' ', sizeof(padded)); | ||
| memcpy(padded, n, nameLen); | ||
| memcpy(padded + DAPLINK_FLASH_FILENAME_LEN, e, extLen); | ||
|
|
||
| _bridge->sendCommand(DAPLINK_FLASH_CMD_SET_FILENAME, padded, sizeof(padded)); | ||
| } |
There was a problem hiding this comment.
fix in 9c6cc64 — setFilename() now returns bool, propagating null-pointer rejection and the bridge sendCommand() result. PR body API box updated to match.
| void setup() { | ||
| Serial.begin(115200); | ||
| internalI2C.begin(); | ||
| if (!flash.begin()) { | ||
| Serial.println("DAPLink Flash not found!"); | ||
| return; | ||
| } | ||
|
|
||
| flash.clearFlash(); | ||
| flash.setFilename("DATA", "CSV"); | ||
| flash.writeLine("temperature,pressure"); | ||
| flash.writeLine("23.5,1013.2"); | ||
| } |
There was a problem hiding this comment.
fix in 9c6cc64 — README quick start now opens with a callout warning that clearFlash() wipes the whole partition, and the snippet itself carries an inline comment flagging the destructive call.
| uint32_t timeoutMs = DAPLINK_BRIDGE_WRITE_TIMEOUT_MS); | ||
|
|
||
| // Issue a command and read up to `maxLen` bytes back. Returns the | ||
| // number of bytes actually read (zero on busy timeout). |
daplink_flash.h includes <daplink_bridge.h>; without a matching `depends=` line in library.properties, PlatformIO LDF can't always chain the include path correctly (its discovery depends on the order in which libs are processed and on whether the consumer's .cpp ends up in compile_commands.json), and Arduino Library Manager won't auto-install daplink_bridge alongside daplink_flash for downstream users. The dep value matches the bridge's own `name=` field exactly. Same pattern documented in CONTRIBUTING.md after the recent library.properties refresh.
The DAPLink family is split across drivers — daplink_bridge speaks the I2C protocol to the F103, and siblings (daplink_flash, daplink_config) compose their own commands on top. Today the bridge only exposes config-zone helpers publicly; everything else (writeFrame, readBlock, error, waitNotBusy) is private. Add two public primitives so siblings can build their own protocol without re-implementing the framing or the busy/error bookkeeping: bool sendCommand(cmd, payload, payloadLen, timeoutMs); size_t readResponse(cmd, buf, maxLen, timeoutMs); sendCommand frames `[cmd | payload...]`, waits for not-busy, sends, waits again, and reports the device error register status. readResponse issues the command then reads back up to maxLen bytes, chunking through the protocol's MAX_READ_CHUNK with the same cursor assumption readConfig already uses. clearConfig/writeConfig/readConfig stay as convenience wrappers — they weren't reimplemented on top of the new primitives in this commit to keep the diff focused, but a follow-up could.
The bridge driver landed on main as `DaplinkBridge` (PascalCase) with
private framing primitives. This commit migrates DaplinkFlash so it
compiles against the current main:
- daplink_bridge -> DaplinkBridge (type + include).
- _bridge->writeTo / writeCmd / readBlock -> sendCommand / readResponse
(the new public primitives added to the bridge).
- _bridge->error() / waitBusy() drop out — the new sendCommand already
bundles them and surfaces the result via its bool return.
- daplink_flash_const.h: re-enable MAX_WRITE_CHUNK and SECTOR_SIZE
(Aline had commented them out when she moved away from the old bridge
const header).
- library.properties: fix depends= casing to match the bridge's
name= ("STeaMi DAPLink Bridge", with the right capitals).
- tests: same type/include update, plus prefixed register names
(DAPLINK_BRIDGE_REG_STATUS / _REG_ERROR / _STATUS_BUSY /
_ERROR_CMD_FAILED).
Two read-path tests (test_read_stops_at_sentinel,
test_read_limited_by_maxlen) are temporarily disabled with a TODO:
the underlying readSector never passes the sector index to the bridge,
so they segfault under the chunked readResponse semantics. Aline will
fix the protocol in her follow-up.
Strictly a compilation fix — the convention issues from the orientation
review (begin(), prefixed flash constants, smoke-test wiring, write/
read API shape, test patterns) stay as her pedagogical to-do list.
…isters. The flash read path previously reused the command opcode as the base register for streamed responses. This caused two protocol issues in native tests: sendCommand() writes were overwriting the mocked read payload, and readResponse() was repeatedly restarting reads from the same register window on each 16-byte chunk. Introduce a dedicated DAPLINK_BRIDGE_REG_RESPONSE stream area so command writes and response reads no longer collide. Refactor DaplinkBridge::readResponse() to consume data from the response stream with deterministic chunk offsets, and keep DaplinkFlash::readSector() on a single buffered 256-byte read instead of per-byte pseudo-register polling. Update native daplink_flash tests to preload mocked read data from the new response register range and fix the clearFlash command assertion so the suite validates the actual command emission. This restores a coherent I2C framing model between bridge commands, streamed payload reads and host-side TwoWire mocks.
clearFlash() returned void and silently swallowed any bridge-side failure (busy timeout or error register set), so a wipe that didn't land would look identical to a successful one to the caller. Return bool and propagate the result of _bridge->sendCommand(...) — same shape as write() / readSector() in the rest of the driver. Also align the README rows for clearFlash() and readSector() with the real signatures (both were still documented as returning void). Add a native test that exercises the new error path (CMD_FAILED in the bridge's error register → clearFlash() returns false).
Mirror what test_hts221 and test_wsen_hids ship for the other on-board drivers, split along the destructive boundary so `make test-hardware` stays safe to run on a board with real flash content: - tests/hardware/test_daplink_flash/: non-destructive smoke checks only (begin() round-trips, getFilename() returns printable ASCII). - tests/integration/test_daplink_flash/: full destructive cycle (clearFlash + setFilename + writeLine + readUntilSentinel + readSector against a known pattern). Wipes the partition — run only when there is nothing to keep on the DAPLink flash. Confirmed on the board: the two hardware tests pass against the current smoke-test firmware (8/8 PASS across the hardware suite). The integration suite is left for the maintainer to run consciously because of the wipe.
Running tests/integration/test_daplink_flash/ on the real STeaMi puts the on-board DAPLink F103 into maintenance mode and the user has to manually re-flash the interface firmware to recover. Root cause is in the DAPLink firmware's handling of CMD_CLEAR_FLASH (it clears sectors beyond the user-data partition), not in the driver itself, but the upshot is the same: shipping this integration suite means anyone running make test-integration loses their DAPLink firmware. Pull the test until the firmware contract is tightened upstream. The non-destructive hardware smoke (tests/hardware/test_daplink_flash/) stays in place — it exercises begin() and the getFilename() read path without touching flash data. Follow-up tracked in a steamicc/DAPLink issue.
Re-selecting REG_RESPONSE + produced between chunks wrapped at 0xFF (0x82 + 0x7D = 0xFF, then 0x00) and silently steered later bytes to the wrong register. Calling waitNotBusy() between chunks reset the firmware response cursor for the same reason. Emit the cmd byte once, then stream the response with successive requestFrom() calls, matching the firmware's TX-buffer cursor semantics.
- setFilename() now returns bool so callers can detect a null pointer or a bridge-side error instead of swallowing it silently. - Native test off-by-one: writes.size() >= 11 (was 10) so writes[10] is in range when validating the padded "TEST" + "TXT" payload. - Hardware setUp() now asserts flash.begin() so a missing bridge fails fast instead of letting downstream assertions fire confusing messages. - README quick-start now warns that clearFlash() wipes the whole partition before showing the call. - Native Wire mock: pre-loaded response payloads land in a dedicated queue, surviving the sendCommand payload writes that previously stomped the register space at the same offset. Pointer-select single-byte transmissions no longer pollute the commands log on subsequent requestFrom().
nedseb
force-pushed
the
feat/daplink-flash-driver
branch
from
7108328 to
9c6cc64
Compare
readResponse() previously took a cmd parameter and re-emitted that cmd byte to "select" the response stream, which the firmware could legitimately interpret as a fresh command — wiping the response buffer on a clean cmd, or hitting BAD_PARAM on a payload-required cmd. The dedicated DAPLINK_BRIDGE_REG_RESPONSE register (0x82) was defined for exactly this stream selection but was never used. Drop the cmd parameter and read from REG_RESPONSE instead. Callers now do sendCommand(cmd, [payload]) followed by readResponse(buf, len) so command actuation and response streaming use the right register each. readResponse() also waits for not-busy, checks the error register, and propagates a non-zero endTransmission() up the stack instead of returning a silent partial read. flash side: getFilename() and readSector() now follow the sendCommand/readResponse split, and write() returns the number of bytes actually committed to flash on a mid-stream chunk failure (was 0) so callers can distinguish a clean failure from a partial write before retrying — relevant because the on-device write is append-only and not atomic. Mock side: setResponse() doc updated to reflect that it keys on a selector register (REG_RESPONSE in practice), not a cmd.
clearFlash() is currently known to brick STeaMi boards into DAPLink maintenance mode (steamicc/DAPLink#9), so the README quick start should not invoke it on every setup(). Drop the call from the quick-start snippet and move the destructive contract into its own "Destructive operations" section with the bricking warning, the upstream issue link, and a gated-call example. API table now cross-links to that section and write() docs the partial-write semantics introduced in the previous commit. Also retarget the hardware-test header comment: the integration suite it pointed at (tests/integration/test_daplink_flash/) was reverted in 3f522e2 for the same DAPLink#9 reason, so calling it "lives in tests/integration" sent maintainers chasing a phantom file.
|
@DumontALINE, récap des modifications côté review depuis ton dernier commit ( Tests + revert intégration
Wave 1 : bug critique de framing I2C
Wave 2 : 7 fixes Copilot (commit
|
- test_read_sector_streams_full_256_bytes_contiguously: verifies a 256-byte sector round-trips byte-for-byte. Catches a regression to the REG_RESPONSE + produced recompute (which wrapped at 0xFF) or any code path that re-selects the read pointer between MAX_READ_CHUNK chunks. - test_write_returns_partial_offset_on_midstream_failure: schedules REG_ERROR after the 2nd chunk so the 1st succeeds and the 2nd fails, then asserts write() returns one chunk's worth of bytes instead of 0. - test_set_filename_rejects_null_name / null_ext / propagates_bridge_error / returns_true_on_success: lock in the new bool return surface for setFilename. Wire mock: new setRegisterAfterNWrites() helper so tests can stage a device-side state change (e.g. flip REG_ERROR) mid-stream after a counted number of multi-byte transactions.
|
@DumontALINE un complément au récap précédent : pour chaque bug corrigé, j'ai ajouté un test de non-régression natif. Le commit dédié est Couverture par bug :
J'en ai profité pour ajouter à Total : 101/101 tests natifs verts, CI verte sur la branche. |
nedseb
changed the title
Goal
Add the high-level file-IO driver layered on
DaplinkBridge(merged in #161). The DAPLink firmware exposes the on-board SPI flash as a FAT-style file system over I²C; this PR wraps that protocol into an Arduino driver matching the MicroPython sister API.Closes #12.
Closes #32.
Partial #44 — the non-destructive hardware smoke is included; the destructive integration suite is deferred (see Known limitation below).
What's in the PR
lib/daplink_flash/— the driver itself (DaplinkFlashclass), register constants, README with destructive-ops guidance, library metadata, keywords.lib/daplink_bridge/— two cross-driver primitives (sendCommand+readResponse) and a dedicated response-stream register (REG_RESPONSE = 0x82) so command writes and response reads use distinct I²C addresses.tests/native/test_daplink_flash/— 19 host-side unit tests against theTwoWiremock. Covers happy paths plus regression locks for the three review-found bugs (multi-chunk read past the 0xFF wrap, partial-write offset semantics,setFilenamebool surface).tests/hardware/test_daplink_flash/— 2 non-destructive smoke tests (begin()+ printable-ASCIIgetFilename()); runs as part ofmake test-hardwarewithout touching flash content.src/main.cpp— driver wired into the smoke-test sketch alongside the other on-board drivers (per tooling: Extend smoke-test sketch to cover every implemented driver #166).Public API (highlights)
bool begin()falseif absent or WHO_AM_I mismatch.bool setFilename(name, ext)falseon null arg or bridge error.FilenameResult getFilename()bool clearFlash()size_t write(data, length)bool readSector(sector, buf)size_t readUntilSentinel(buf, max)Every method that talks to the bridge propagates the bridge's busy-timeout and error register:
false/0on failure, deterministic success otherwise. See the README for the full surface and call patterns.Known limitation —
clearFlash()on real hardwareclearFlash()is known to brick STeaMi boards into DAPLink maintenance mode. The I²C transaction is byte-identical to the working MicroPythonclear_flash(), and the F103 firmware is supposed to wipe only the external W25Q64 (not the F103 internal flash that holds the interface firmware), so the bug sits in the firmware itself. Tracked insteamicc/DAPLink#9.Until that lands:
clearFlash()stays in the public API (parity with MicroPython); the README opens with a⚠️ Destructive operationssection and the quick-start does not call it.2bb70f5, removed in3f522e2); shipping it would mean everymake test-integrationwipes the interface firmware. Re-add once DAPLink#9 lands.Follow-ups
daplink_flash.{h,cpp,_const.h}→DaplinkFlash.{h,cpp,_const.h}(PascalCase, matchingDaplinkBridge.h,HTS221.h, etc.). Deferred to keep this PR focused on runtime fixes.Test plan
make ci(lint + format-check + native) — 101/101 native PASSmake test-hardwareon a wired STeaMi — daplink_flash smoke PASSmake build(steami env) — smoke sketch links and runsdaplink_flash.begin()on the boardmake test-integration— deferred to after DAPLink#9