feat(mcu-util): use polarizer state feedback #928
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances polarizer command reliability by waiting for polarizer state feedback messages to confirm successful wheel movement, rather than relying solely on command acknowledgment.
Changes:
- Added
wait_for_polarizer_wheel_state()method to wait for polarizer state feedback - Modified polarizer commands to wait for state confirmation after receiving ACK
- Updated polarizer stress test to validate state changes with 1-second delays between commands
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| match tokio::time::timeout( | ||
| Duration::from_secs(2), | ||
| self.wait_for_polarizer_wheel_state(), | ||
| ) | ||
| .await | ||
| { | ||
| Ok(Ok(state)) => { | ||
| info!("💈 Polarizer command {:?}: success", opts); | ||
| debug!("Polarizer wheel state: {:?}", state); | ||
| } | ||
| Ok(Err(e)) => { | ||
| return Err(eyre!( | ||
| "Error waiting for PolarizerWheelState for command {:?}: {:?}", | ||
| opts, | ||
| e | ||
| )); | ||
| } | ||
| Err(_) => { | ||
| return Err(eyre!( | ||
| "Timeout waiting for PolarizerWheelState message for command {:?}", | ||
| opts | ||
| )); | ||
| } | ||
| } |
There was a problem hiding this comment.
The code waits for any PolarizerWheelState message without validating that the state actually matches the requested command. For example, if the command was to move to "Horizontal" position, the code doesn't verify that the received state indicates the wheel is in the horizontal position. This means a command could be considered successful even if the wheel didn't actually move to the requested position. Consider adding validation to ensure the received state matches the expected state for the command that was sent.
| match tokio::time::timeout( | ||
| Duration::from_secs(2), | ||
| self.wait_for_polarizer_wheel_state(), | ||
| ) | ||
| .await | ||
| { | ||
| Ok(Ok(state)) => { | ||
| success_count += 1; | ||
| debug!( | ||
| "[{}/{}] Polarizer -> {}: success [{:?}]", | ||
| i + 1, | ||
| repeat, | ||
| name, | ||
| state | ||
| ); | ||
| } | ||
| Ok(Err(e)) => { | ||
| error_count += 1; | ||
| error!( | ||
| "[{}/{}] Polarizer -> {}: error waiting for state {:?}", | ||
| i + 1, | ||
| repeat, | ||
| name, | ||
| e | ||
| ); | ||
| } | ||
| Err(_) => { | ||
| error_count += 1; | ||
| error!( | ||
| "[{}/{}] Polarizer -> {}: timeout waiting for state", | ||
| i + 1, | ||
| repeat, | ||
| name | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Similar to the polarizer method, this code waits for any PolarizerWheelState message without validating that the state actually corresponds to the requested position. The stress test should verify that the wheel actually reached the commanded position (passthrough, vertical, or horizontal) before counting it as a success. Otherwise, the test may report false successes when the wheel didn't actually move to the requested position.
| async fn wait_for_polarizer_wheel_state( | ||
| &mut self, | ||
| ) -> Result<main_messaging::PolarizerWheelState> { | ||
| loop { | ||
| let Some(mcu_payload) = self.message_queue_rx.recv().await else { | ||
| return Err(eyre!("message queue closed")); | ||
| }; | ||
| let McuPayload::FromMain(main_mcu_payload) = mcu_payload else { | ||
| continue; | ||
| }; | ||
|
|
||
| if let main_messaging::mcu_to_jetson::Payload::PolarizerWheelState(state) = | ||
| main_mcu_payload | ||
| { | ||
| return Ok(state); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The wait_for_polarizer_wheel_state method consumes all messages from the message queue until it finds a PolarizerWheelState message, discarding any other messages that arrive (line 683 continues on non-matching messages). This could cause message loss if other important messages arrive during the wait period.
If a PolarizerWheelState message was already sent before this method is called (e.g., from a previous command), it will immediately consume that stale state instead of waiting for the new state corresponding to the current command. This creates a race condition where you might receive the state from a previous operation rather than the current one.
Consider one of these approaches:
- Clear any pending
PolarizerWheelStatemessages before sending the command - Add timestamp or sequence number validation to ensure the state corresponds to the current command
- Use a broadcast/watch channel pattern where state updates don't consume messages needed by other parts of the system
There was a problem hiding this comment.
In orb-mcu-util utility, command is run by the user, and during that command execution user doesn't care about other subsystems. So discarding other message types here is intended.
Correct @fouge ?
| // delay 1 second between commands | ||
| tokio::time::sleep(Duration::from_secs(1)).await; |
There was a problem hiding this comment.
The delay has been moved to occur before waiting for the send result (line 414), which means it now delays before checking if the send was successful. This changes the timing semantics - previously, the delay occurred after a successful send. If the send fails, the delay still occurs, which adds unnecessary wait time when errors happen. Consider moving the delay to after the successful state verification (after line 484) to maintain the original semantics of delaying between successful operations.
Qbicz
left a comment
Qbicz
left a comment
There was a problem hiding this comment.
Looks good, please check my question
| async fn wait_for_polarizer_wheel_state( | ||
| &mut self, | ||
| ) -> Result<main_messaging::PolarizerWheelState> { | ||
| loop { | ||
| let Some(mcu_payload) = self.message_queue_rx.recv().await else { | ||
| return Err(eyre!("message queue closed")); | ||
| }; | ||
| let McuPayload::FromMain(main_mcu_payload) = mcu_payload else { | ||
| continue; | ||
| }; | ||
|
|
||
| if let main_messaging::mcu_to_jetson::Payload::PolarizerWheelState(state) = | ||
| main_mcu_payload | ||
| { | ||
| return Ok(state); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
In orb-mcu-util utility, command is run by the user, and during that command execution user doesn't care about other subsystems. So discarding other message types here is intended.
Correct @fouge ?
fouge
force-pushed
the
fouge/mcu-util-polarizer-state
branch
from
32a999a to
3f81244
Compare
use polarizer state message as an indication of completion the message is sent on any new state & thus can be used to ensure the wheel moved successfully as per the request in the polarizer commands
fouge
force-pushed
the
fouge/mcu-util-polarizer-state
branch
from
3f81244 to
677898f
Compare
use polarizer state message as an indication of completion the message is sent on any new state & thus can be used to ensure the wheel moved successfully as per the request in the polarizer commands