Add support for continuous reporting #802
Conversation
WalkthroughAdds Control.__log_indState to decode and emit structured logs for API6 IndState tokens and routes indState handling through it. Engine FSM change makes API6 start requests force continuous reporting when AYAB_DEBUG is set; otherwise preserves prior behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant EngineFSM as EngineFSM
participant Control as Control
participant Comms as Comms
Note over EngineFSM,Comms: API6 start request (debug override)
User->>EngineFSM: initiate start
EngineFSM->>EngineFSM: check AYAB_DEBUG env
alt AYAB_DEBUG set
EngineFSM->>Comms: req_start_API6(..., continuous_reporting=True)
else AYAB_DEBUG not set
EngineFSM->>Comms: req_start_API6(..., continuous_reporting=control.continuous_reporting)
end
sequenceDiagram
autonumber
participant Serial as SerialPort
participant Control as Control
participant Logger as Logger
Note over Serial,Control: API6 token processing for IndState
Serial-->>Control: Token.indState + payload
Control->>Control: __log_indState(payload)
Control->>Logger: structured IndState info log (mapped fields)
alt optional params missing
Control->>Logger: reduced/fallback log format
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/python/main/ayab/engine/engine_fsm.py (1)
170-177: Keep status.active in sync with forced continuous reportingWhen
AYAB_DEBUGis set you now force continuous reporting on the wire, butstatus.activeremains tied tocontrol.continuous_reporting. That leaves the UI/engine believing reporting is off while the device streams continuously. Please derive a singleeffective_continuous_reportingvalue and use it for bothstatus.activeand thereq_start_API6call.- control.status.active = control.continuous_reporting - control.com.req_start_API6( + debug_reporting = bool(os.getenv("AYAB_DEBUG")) + effective_continuous_reporting = ( + True if debug_reporting else control.continuous_reporting + ) + control.status.active = effective_continuous_reporting + control.com.req_start_API6( control.pattern.knit_start_needle, control.pattern.knit_end_needle - 1, - True if os.getenv("AYAB_DEBUG") else control.continuous_reporting, + effective_continuous_reporting,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/python/main/ayab/engine/control.py(2 hunks)src/main/python/main/ayab/engine/engine_fsm.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/python/main/ayab/engine/engine_fsm.py (3)
src/main/python/main/ayab/engine/communication_mock.py (1)
req_start_API6(97-103)src/main/python/main/ayab/tests/test_communication_mock.py (2)
test_req_line_API6(97-115)test_req_start_API6(44-56)src/main/python/main/ayab/engine/communication.py (1)
req_start_API6(137-156)
src/main/python/main/ayab/engine/control.py (2)
src/main/python/main/ayab/engine/communication.py (1)
Token(39-66)src/main/python/main/ayab/signal_sender.py (1)
emit_hw_test_writer(102-103)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Create and upload Mac OSX build
- GitHub Check: Create and upload Windows build
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/python/main/ayab/engine/control.py (1)
180-183: Consider defining mapping dictionaries as class constants.The mapping dictionaries are recreated on every call to
__log_indState. Moving them to class-level constants (e.g.,_DIRECTION_MAP,_CARRIAGE_MAP, etc.) would avoid repeated allocation and improve readability by making these mappings more discoverable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/python/main/ayab/engine/control.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/python/main/ayab/engine/control.py (3)
src/main/python/main/ayab/engine/communication.py (1)
Token(39-66)src/main/python/main/ayab/signal_sender.py (1)
emit_hw_test_writer(102-103)src/main/python/main/ayab/engine/status.py (1)
parse_device_state_API6(177-211)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Create and upload Mac OSX build
- GitHub Check: Create and upload Windows build
🔇 Additional comments (1)
src/main/python/main/ayab/engine/control.py (1)
189-189: Previous KeyError concerns addressed.The use of
.get()with hex fallbacks (e.g.,f"0x{msg[7]:02x}?") successfully prevents KeyError exceptions for unexpected firmware codes, as requested in the previous review.Also applies to: 191-191, 194-195
Log the contents of the indState message to the console Continuous reporting remains disabled by default and can be enabled by setting the AYAB_DEBUG environment variable
jpcornil-git
force-pushed
the
continuousReporting
branch
from
b3c57fd to
8d15874
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/python/main/ayab/engine/control.py(2 hunks)src/main/python/main/ayab/engine/engine_fsm.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/python/main/ayab/engine/engine_fsm.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/python/main/ayab/engine/control.py (3)
src/main/python/main/ayab/engine/communication.py (1)
Token(39-66)src/main/python/main/ayab/signal_sender.py (1)
emit_hw_test_writer(102-103)src/main/python/main/ayab/engine/status.py (1)
parse_device_state_API6(177-211)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Create and upload Mac OSX build
- GitHub Check: Create and upload Windows build
🔇 Additional comments (2)
src/main/python/main/ayab/engine/control.py (2)
42-42: LGTM!The
structimport is necessary for decoding binary fields in IndState messages and is properly placed with other imports.
173-173: LGTM!The logging call is appropriately placed after state parsing and adds observability without altering the existing data flow.
| def __log_indState(self, msg: bytes) -> None: | ||
| try: | ||
| directionMap = {0x00: "Left ", 0x01: "Right", 0xFF: "?"} | ||
| machineSideMap = {0x00: "? ", 0x01: "Left", 2: "Right"} | ||
| beltShiftMap = {0x00: "?", 0x01: "Regular", 0x02: "Shifted"} | ||
| carriageMap = {0x00: "K", 0x01: "L", 0x02: "G", 0x03: "K270", 0xFF: "None"} | ||
|
|
||
| error = msg[1] | ||
| state = msg[2] | ||
| hallLeft = struct.unpack(">H", msg[3:5])[0] | ||
| hallRight = struct.unpack(">H", msg[5:7])[0] | ||
| carriage = carriageMap.get(msg[7], f"0x{msg[7]:02x}?") | ||
| position = msg[8] | ||
| direction = directionMap.get(msg[9], f"0x{msg[9]:02x}?") | ||
|
|
||
| try: # AyabAsync supplies additional parameters | ||
| hallActive = machineSideMap.get(msg[10], f"0x{msg[10]:02x}?") | ||
| beltShift = beltShiftMap.get(msg[11], f"0x{msg[11]:02x}?") | ||
| self.logger.info( | ||
| f"IndState: {error:1d} {state:1d} {position:3d}" | ||
| f" {carriage:5s} ({beltShift:11s},{direction:5s})" | ||
| f" Hall:{hallActive:5s} ({hallLeft:5d}, {hallRight:5d})" | ||
| ) | ||
| except IndexError: | ||
| self.logger.info( | ||
| f"IndState: {error:1d} {state:1d} {position:3d}" | ||
| f" {carriage:5s} ({direction:5s})" | ||
| f" Hall:({hallLeft:5d}, {hallRight:5d})" | ||
| ) | ||
| except (IndexError, struct.error) as e: | ||
| self.logger.warning(f"Unable to parse InState message: {e}") | ||
|
|
There was a problem hiding this comment.
Fix typo in warning message.
The error handling and logging logic are well-structured with appropriate fallbacks for unknown codes and resilience against malformed messages. However, there's a typo in the warning message.
Apply this diff to correct the typo:
- self.logger.warning(f"Unable to parse InState message: {e}")
+ self.logger.warning(f"Unable to parse IndState message: {e}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def __log_indState(self, msg: bytes) -> None: | |
| try: | |
| directionMap = {0x00: "Left ", 0x01: "Right", 0xFF: "?"} | |
| machineSideMap = {0x00: "? ", 0x01: "Left", 2: "Right"} | |
| beltShiftMap = {0x00: "?", 0x01: "Regular", 0x02: "Shifted"} | |
| carriageMap = {0x00: "K", 0x01: "L", 0x02: "G", 0x03: "K270", 0xFF: "None"} | |
| error = msg[1] | |
| state = msg[2] | |
| hallLeft = struct.unpack(">H", msg[3:5])[0] | |
| hallRight = struct.unpack(">H", msg[5:7])[0] | |
| carriage = carriageMap.get(msg[7], f"0x{msg[7]:02x}?") | |
| position = msg[8] | |
| direction = directionMap.get(msg[9], f"0x{msg[9]:02x}?") | |
| try: # AyabAsync supplies additional parameters | |
| hallActive = machineSideMap.get(msg[10], f"0x{msg[10]:02x}?") | |
| beltShift = beltShiftMap.get(msg[11], f"0x{msg[11]:02x}?") | |
| self.logger.info( | |
| f"IndState: {error:1d} {state:1d} {position:3d}" | |
| f" {carriage:5s} ({beltShift:11s},{direction:5s})" | |
| f" Hall:{hallActive:5s} ({hallLeft:5d}, {hallRight:5d})" | |
| ) | |
| except IndexError: | |
| self.logger.info( | |
| f"IndState: {error:1d} {state:1d} {position:3d}" | |
| f" {carriage:5s} ({direction:5s})" | |
| f" Hall:({hallLeft:5d}, {hallRight:5d})" | |
| ) | |
| except (IndexError, struct.error) as e: | |
| self.logger.warning(f"Unable to parse InState message: {e}") | |
| def __log_indState(self, msg: bytes) -> None: | |
| try: | |
| directionMap = {0x00: "Left ", 0x01: "Right", 0xFF: "?"} | |
| machineSideMap = {0x00: "? ", 0x01: "Left", 2: "Right"} | |
| beltShiftMap = {0x00: "?", 0x01: "Regular", 0x02: "Shifted"} | |
| carriageMap = {0x00: "K", 0x01: "L", 0x02: "G", 0x03: "K270", 0xFF: "None"} | |
| error = msg[1] | |
| state = msg[2] | |
| hallLeft = struct.unpack(">H", msg[3:5])[0] | |
| hallRight = struct.unpack(">H", msg[5:7])[0] | |
| carriage = carriageMap.get(msg[7], f"0x{msg[7]:02x}?") | |
| position = msg[8] | |
| direction = directionMap.get(msg[9], f"0x{msg[9]:02x}?") | |
| try: # AyabAsync supplies additional parameters | |
| hallActive = machineSideMap.get(msg[10], f"0x{msg[10]:02x}?") | |
| beltShift = beltShiftMap.get(msg[11], f"0x{msg[11]:02x}?") | |
| self.logger.info( | |
| f"IndState: {error:1d} {state:1d} {position:3d}" | |
| f" {carriage:5s} ({beltShift:11s},{direction:5s})" | |
| f" Hall:{hallActive:5s} ({hallLeft:5d}, {hallRight:5d})" | |
| ) | |
| except IndexError: | |
| self.logger.info( | |
| f"IndState: {error:1d} {state:1d} {position:3d}" | |
| f" {carriage:5s} ({direction:5s})" | |
| f" Hall:({hallLeft:5d}, {hallRight:5d})" | |
| ) | |
| except (IndexError, struct.error) as e: | |
| self.logger.warning(f"Unable to parse IndState message: {e}") |
🤖 Prompt for AI Agents
In src/main/python/main/ayab/engine/control.py around lines 179 to 210, the
exception warning string contains a typo ("InState") that should match the
logged message prefix ("IndState"); update the logger.warning call to use
"IndState" instead of "InState" so the warning reads f"Unable to parse IndState
message: {e}" (preserve the f-string and exception variable).
8d15874 to
53db14e
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/python/main/ayab/engine/control.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/python/main/ayab/engine/control.py (2)
src/main/python/main/ayab/engine/communication.py (1)
Token(39-66)src/main/python/main/ayab/signal_sender.py (1)
emit_hw_test_writer(102-103)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Create and upload Linux AppImage release
- GitHub Check: Create and upload Windows build
- GitHub Check: Create and upload Mac OSX build
| except (IndexError, struct.error) as e: | ||
| self.logger.warning(f"Unable to parse indState message: {e}") |
There was a problem hiding this comment.
Fix casing in the warning message.
The warning still prints indState, which makes log filtering inconsistent with the IndState prefix used elsewhere. Please match the casing.
- self.logger.warning(f"Unable to parse indState message: {e}")
+ self.logger.warning(f"Unable to parse IndState message: {e}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except (IndexError, struct.error) as e: | |
| self.logger.warning(f"Unable to parse indState message: {e}") | |
| except (IndexError, struct.error) as e: | |
| self.logger.warning(f"Unable to parse IndState message: {e}") |
🤖 Prompt for AI Agents
In src/main/python/main/ayab/engine/control.py around lines 208-209, the warning
message prints "indState" with wrong casing; update the f-string to use
"IndState" (e.g. self.logger.warning(f"Unable to parse IndState message: {e}"))
so the log message matches the existing "IndState" prefix used elsewhere.
Log the contents of the indState message to the console Continuous reporting remains disabled by default and can be enabled by setting the AYAB_DEBUG environment variable
Summary by CodeRabbit
New Features
Refactor