tooldb: notify db_program of G10 tool offsets - #4422
Conversation
In db mode no tool table file is written, so offsets set with G10 L1/L10/L11 were lost at restart without the TOOL_OFFSET notification.
It already provides the estop and toolchange loopbacks, so the local copy is not needed; drop the .gitignore entries covered by tests/.gitignore.
|
Done |
grandixximo
left a comment
grandixximo
left a comment
There was a problem hiding this comment.
Verified against the tree, this looks right to me.
Some history on the commented-out block: it was active code in the old ioControl.cc and was commented out (with the stale io_db_mode name) by 764655e "moving all IO handling from iocontrol to task" during the taskio migration. So there was no deliberate reason to disable it, it was a casualty of that move. And the bug is real: in db mode with a nonrandom toolchanger tooldata_save() returns early without writing anything (tooldata_common.cc), so offsets set with G10 were lost at restart without this notification.
The pno logic matches the pre-move code, db_mode is in scope, and the 'p' notification is synchronous (send_and_verify), so the wait_complete() ordering in the test is sound. The test covers restart persistence, the loaded-tool case, and survival of untouched tools. Nice work.
One non-blocking nit inline.
| @@ -0,0 +1,6 @@ | |||
| Verify that tool offsets set with G10 L1/L10/L11 are persisted when an | |||
There was a problem hiding this comment.
Nit (non-blocking): the README and PR body say L1/L10/L11, but the test only exercises L1. Same code path, but L10/L11 cases would be cheap to add.
3 participants
In db mode no tool table file is written, so offsets set with G10 L1/L10/L11 were lost at restart without the TOOL_OFFSET notification.
may be there was good reason to comment these lines, but now it seems to work ok. I added a test for it.