Skip to content

Don't fail on ack comms error if validated response has been received#137

Merged
marionbarker merged 2 commits into
LoopKit:devfrom
itsmojo:improved-PodMessageTransport-error-handling
Dec 10, 2024
Merged

Don't fail on ack comms error if validated response has been received#137
marionbarker merged 2 commits into
LoopKit:devfrom
itsmojo:improved-PodMessageTransport-error-handling

Conversation

@itsmojo
Copy link
Copy Markdown

@itsmojo itsmojo commented Nov 21, 2024

  • make readAndAckResponse func as private
  • comment out a mostly reductant and unneeded log.debug statement

@itsmojo
Copy link
Copy Markdown
Author

itsmojo commented Dec 6, 2024

This code was tested by introducing fake comms errors originally at a low Bluetooth layer (PeripheralManager.runCommand()) and then later in MessageTransport.readAndAckResponse(). A flag was added to control whether to throw PodProtocolError.messageIOException as before or to just log the error and return the response if an Omnipod command message had been sent and a validated response received when the final ACK has some comms error. When faking ACK comms errors both at the low and high levels, it was found that Loop could continue and use the response instead of potentially having to go into unacknowledgedCommand mode is an exception is raised. Tests runs were done against both actual Dash pods and the rPi pod sim. On the next Omnipod message following a faked ACK comms error, actual Dash pods generally recovered immediately while the pod sim would exit and would need to be restarted.

@marionbarker
Copy link
Copy Markdown
Collaborator

Status

LGTM

Code Review

The code review looks good. Relying on the work @itmojo did by inserting errors.

@marionbarker marionbarker merged commit e1ec43c into LoopKit:dev Dec 10, 2024
@itsmojo itsmojo deleted the improved-PodMessageTransport-error-handling branch September 12, 2025 05:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants