feat: Q10 (B01/ss07) clean-record history trait#857
Conversation
andrewlyeats
force-pushed
the
feat/q10-clean-history
branch
from
80994f3 to
80b0bd6
Compare
allenporter
left a comment
allenporter
left a comment
There was a problem hiding this comment.
Thank you @andrewlyeats this is fantastic. I have some code structure related comments, though overall look forward to merging this feature.
| _E = TypeVar("_E", bound=IntEnum) | ||
|
|
||
|
|
||
| def _safe_clean_enum(enum_cls: type[_E], value: int | None) -> _E | None: |
There was a problem hiding this comment.
Can you take a look at the enum definitions in code_mappings and use them above? I believe it will give you equivalent functionality to what is needed here, but i didn't look really closely.
|
|
||
| The device returns each record as an underscore-delimited string in the ``data`` | ||
| list of a ``{"op": "list"}`` query. The field names and meanings here follow the | ||
| device's own app, which splits the string into these 12 values. The ``*_len`` |
There was a problem hiding this comment.
don't need to mention device app here, just say its how the format works
| """Length of the saved path blob for this record (0 = none stored).""" | ||
| virtual_len: int | None = None | ||
| """Length of the saved virtual-restriction blob for this record (0 = none stored).""" | ||
| clean_mode: int | None = None |
There was a problem hiding this comment.
Once you update the fields above it should be possible to just directly add the enum types here and RoborockBase should parse them correctly, so you don't need the additional enum property method converters.
| params={str(B01_Q10_DP.CLEAN_RECORD.code): {"op": "list"}}, | ||
| ) | ||
|
|
||
| def update_from_dps(self, decoded_dps: dict[B01_Q10_DP, Any]) -> None: |
There was a problem hiding this comment.
Can you review the converter pattern used in other objects and extract separate objects for parsing the data vs updating state on the object? I realize this is more complex since it supports a merge vs an update, but i do think we still want to separate concerns a little bit more here. The trait can manage merge vs update still.
We can simplify this so we don't need two copies of the code that rebuilds the list, sorts it, and notifies, since that can happen in one place. So i think that would be:
- define a format for the notification
- have a class for parsing the dps to that object
- the trait then will get the object, decide if its merging or updating (single if statement branch), then sort (one impementation) and notify (one implementation).
My gut says the string conversion logic belongs in the converter /trait and not in the container.
|
|
||
| The device returns each record as an underscore-delimited string in the ``data`` | ||
| list of a ``{"op": "list"}`` query. The field names and meanings here follow the | ||
| device's own app, which splits the string into these 12 values. The ``*_len`` |
There was a problem hiding this comment.
Can you add some full payload examples in /tests/protocols/testdata/b01_q10_protocol? it serves as nice documentation of strings we actually see in practice from the device, since it can be a little harder to understand the full payload in unit tests
Q10 clean history is push-driven over dpCleanRecord (DP 52), unlike the Q7's
synchronous get_record_list RPC. Adds:
- Q10CleanRecord: parses the 12-field op:list underscore string (raw retained),
with crash-safe enum accessors .scope/.work/.result/.started_by (IntEnums that
return None for an unmapped code, never raising like the YX from_code path)
- CleanHistoryTrait: refresh() sends {op:list}; update_from_dps decodes both the
full list and the single op:notify clean-finished push (upsert by record_id,
newest-first); registered as a read-model trait in the dispatch loop
Field layout is the device app's own (setHoldData), cross-confirmed by ioBroker's
Q10CleanRecordService. Tests cover decode, enum labels + unmapped-safety, and the
list/notify paths.
andrewlyeats
force-pushed
the
feat/q10-clean-history
branch
from
80b0bd6 to
8055598
Compare
|
Thanks — addressed all five, pushed. Enums:
Fields/converters: enums directly, property layer gone. Conversion runs in the converter via Structure: Docs/tests: dropped the app mention; added These "contributions" (I hope they are!) have been a collaboration between a physicist trying to learn about modern coding tools and one of those tools, Claude Code. We both hope our efforts to be gracious and follow open source software etiquette are apparent in our contributions here. If we are in fact injecting noise, please tell us and we will stop! We thank you for your work on this repo! |
2 participants
Implements #767 for B01/ss07 (Q10).
Adds a
CleanHistoryTraitthat fetches and decodes the Q10 clean-record history. Unlike the Q7's synchronousget_record_listRPC, the Q10 is push-driven:refresh()sends{"op":"list"}fordpCleanRecord(DP 52) overdpCommon, and the device publishes its record list back on the subscribe stream.Shape
CleanRecordConverterparses adpCleanRecordenvelope — the fullop:"list"reply or a singleop:"notify"push — into aCleanRecordPush; the trait's_applydoes the merge-or-replace, one sort, one notify.Q10CleanRecord(12 underscore fields), withclean_mode/work_mode/cleaning_result/start_methodtyped as enums viafrom_code_optional(an unmapped code →None; the original stays inraw).Validation (real ss07 Q10, fw 03.11.24)
op:"list"decoded 25 records live on the device's own reply topic; the end-of-cleanop:"notify"push was captured on a genuine whole-apartment completion and upserted correctly.Q10CleanRecordService(independent implementation).tests/protocols/testdata/b01_q10_protocol/.Caveats (one device observed)
clean_mode=2andstart_method=0were never seen on our unit — left unmapped rather than guessed (they surface asNone;rawretains the value).work_modeusesYXCleanType(matchingQ10Status); it caps at 4 while the liveCLEAN_MODEDP can reach 6, though records only ever show 1/2/3.map_lengate) is intentionally out of scope — a natural follow-up.