[Fix] Dual-UID spool identification, use tray_uuid end-to-end#1200
[Fix] Dual-UID spool identification, use tray_uuid end-to-end#1200Keybored02 wants to merge 12 commits into
Conversation
|
Not sure if this is the right moment to add it. Please align with @netscout2001 |
|
Not sure it's possible to defer it. Spoolbuddy is getting traction and the current tag assignment has a duplication issue and does not track the correct NFC block. Later changes will have a much broader impact. |
|
Talking to each other helps ;) |
|
Will do. |
|
Hey, thanks a lot for taking the time to put this together — I really appreciate the effort! 🙏 That said, I don't want to pull this into my current PR at this point. It can be revisited and implemented once I'm done with the Spoolman integration on my side — I'd rather not introduce a second UUID in there right now. I only had a quick look, so this is a fly-by review, but from what I can see the test coverage isn't complete yet — a few tests are in there, but not enough to fully cover the changes. On top of that, this is more of a new feature than only a bug fix. Let's revisit this later once Spoolman is wrapped up. Thanks again for contributing! 🚀 |
|
Thanks for taking the time to review this. This is not a DB feature but rather a fix for an issue in Spoolbuddy. A Bambulab spool has two tags, each with one unique If you think that covering the full behavior is excessive (2 tags and 1 tray IDs per spool), at least add the switch to tray_uuid and the hw config for that. Given that the project is getting quite a bit more users recently, I do not see how this can be postponed. I can extendTesting coverage. |
|
Unfortunately we have to postpone it until PR #1114 is finished. The PR is too complex and I would like to get it finished. I also don't see any pressure with the duplicate tags. Don't experienced any issues with it unti now and also don't received any issues which are related. |
|
BTW: we are talking about BL spools - third party spools don't have the issue. So we have a nice workaround until it's fixed: don't weigh your BL spools, just put them into the AMS. |
I constantly run into this issue. The lack of reporting would probably be for the fact that it's rather hard to catch.
That's not how people are using it though. Anyway, will wait. |
Sure, it's a workaround until the fix lands. |
|
I've just merged the large unified inventory PR. Now it's the perfect time to merge your changes. |
|
Lemme check.... |
|
I'll fix the merging issues, add test coverage, and test the while thing again. Will take a while. |
|
Wait....I'm nearly finished with resolving conflicts. |
maziggy
left a comment
There was a problem hiding this comment.
Three things worth changing before merge plus one nit:
- tag_uid_2 length should be VARCHAR(32), not VARCHAR(16).
PR #1241 widened tag_uid from String(16) to String(32) to support the full Bambu tray UUID width. Adding tag_uid_2 as String(16) reintroduces the old limit on a brand-new column, so the secondary slot can't hold the same kinds of UIDs the primary slot now accepts. I bumped this to String(32) (and the migration ALTER TABLE to VARCHAR(32)) during the merge — please confirm that's what you wanted.
- InventorySpool.tag_uid_2 should be optional, not required.
Making it tag_uid_2: string | null (required) breaks every existing literal that constructs an InventorySpool shape — tsc -b flagged three on dev that the PR didn't touch:
- frontend/src/components/ForecastPanel.tsx:1189 (bulkCreateSpools payload from "received" shopping-list items)
- frontend/src/pages/spoolbuddy/SpoolBuddyDashboard.tsx — the setJustLinkedSpool literal in the Spoolman link branch
- frontend/src/pages/spoolbuddy/SpoolBuddyDashboard.tsx — the createSpoolmanInventorySpool payload in the Spoolman quick-add branch
I added tag_uid_2: null in those three spots so the build passes, but the cleaner long-term fix is tag_uid_2?: string | null on the InventorySpool interface — matches how every other additive column in this codebase has been introduced and avoids the next dev who adds an inventory literal silently breaking the build.
- Spoolman backend has no tag_uid_2 column.
The dual-UID feature is local-DB only — Spoolman's REST API doesn't expose a second tag field, and _map_spoolman_spool won't have anywhere to read one from. So under
spoolman_enabled=true, the secondary UID column is unreachable: setting it via the form persists only locally (and only in local mode), and no Spoolman spool will ever match against it. That's fine as a scoping decision, but worth saying so explicitly in the PR description so users on Spoolman don't expect dual-reader matching to work.
- Nit: setShowLinkModal(false) on the success path of handleLinkTagToSpool is redundant with the existing finally { setShowLinkModal(false) }. Removed during the merge.
|
For 3. it is possible to add a tag2 field as extra field to spoolman. |
|
|
Question is, if this PR is still good after merging the huge "Unified inventory" code. |
|
I've not run into any particular issues with my flow, but I've not tested extensively on Spoolman. I'm also unclear if you still need me to add anything before a review? Seems like you've already fixed the remaining points. |
Description
Adds a secondary
tag_uid_2field so a spool can be identified by two different hardware UIDs (different physical tags). Fixes tray_uuid being silently dropped when adding a new spool through SpoolBuddy — it's now saved and used as the primary match identifier end-to-end. Also fixes the spool card staying visible on the SpoolBuddy dashboard after removing a freshly-added spool.Related Issue
Fixes #984
Documentation
Companion docs PRs (delete lines that don't apply):
Pick one:
Type of Change
Changes Made
Adds an optional secondary hardware UID column per spool, to handle the real-world case where the same physical tag is reported with a different first byte by different NFC reader hardware. The second slot is strictly opt-in — it is never auto-assigned; it can only be set explicitly via the PATCH /spools/{id}/link-tag API or the spool form.
New tag_uid_2 VARCHAR(16) column on the spool table with an inline _safe_execute migration
All matching logic (get_spool_by_tag, fuzzy fallback) searches both columns transparently via a shared _fuzzy_uid_match() helper
Conflict checks in the link-tag route cover both columns on active and archived spools
Frontend displays the primary UID with both in the tooltip; "tagged/untagged" filters and the write-tag page check both columns
Previously, when a Bambu Lab spool was scanned by SpoolBuddy and the user created a new inventory entry or linked it to an existing spool, the tray_uuid (read from MIFARE Classic block 9) was silently dropped. This meant subsequent scans matched by hardware UID only, losing the more-reliable UUID-based matching.
spoolbuddy_unknown_tag and spoolbuddy_tag_matched WebSocket broadcasts now include tray_uuid
useSpoolBuddyState carries tray_uuid through state (unknownTrayUuid, currentTrayUuid)
SpoolBuddyDashboard tracks displayedTrayUuid alongside displayedTagId, and passes it to createSpool and linkTagToSpool calls
tag_type is set to 'bambulab' (not 'generic') when a tray_uuid is present
After a quick-add, removing the spool from the scale/reader left the card visible. The tag-removal useEffect only cleared state when hiddenTagId was set (i.e. the user had closed the card), so a freshly-added spool — where the card was never manually closed — was never cleared on physical removal.
The else-branch now clears displayedTagId/displayedTrayUuid whenever either is non-null, unconditionally
Screenshots
Testing
Checklist
Additional Notes