-
Notifications
You must be signed in to change notification settings - Fork 186
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve ACK handling: pass to onAckNak
and on request in sendData
#602
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -296,6 +296,7 @@ def sendData( | |
wantAck: bool=False, | ||
wantResponse: bool=False, | ||
onResponse: Optional[Callable[[dict], Any]]=None, | ||
onResponseAckPermitted: bool=False, | ||
channelIndex: int=0, | ||
): | ||
"""Send a data packet to some other node | ||
|
@@ -315,6 +316,10 @@ def sendData( | |
onResponse -- A closure of the form funct(packet), that will be | ||
called when a response packet arrives (or the transaction | ||
is NAKed due to non receipt) | ||
onResponseAckPermitted -- should the onResponse callback be called | ||
for regular ACKs (True) or just data responses & NAKs (False) | ||
Note that if the onResponse callback is called 'onAckNak' this | ||
will implicitly be true. | ||
channelIndex - channel number to use | ||
|
||
Returns the sent packet. The id field will be populated in this packet | ||
|
@@ -346,7 +351,7 @@ def sendData( | |
|
||
if onResponse is not None: | ||
logging.debug(f"Setting a response handler for requestId {meshPacket.id}") | ||
self._addResponseHandler(meshPacket.id, onResponse) | ||
self._addResponseHandler(meshPacket.id, onResponse, ackPermitted=onResponseAckPermitted) | ||
p = self._sendPacket(meshPacket, destinationId, wantAck=wantAck) | ||
return p | ||
|
||
|
@@ -528,8 +533,8 @@ def onResponseTelemetry(self, p: dict): | |
if p["decoded"]["routing"]["errorReason"] == 'NO_RESPONSE': | ||
our_exit("No response from node. At least firmware 2.1.22 is required on the destination node.") | ||
|
||
def _addResponseHandler(self, requestId: int, callback: Callable[[dict], Any]): | ||
self.responseHandlers[requestId] = ResponseHandler(callback) | ||
def _addResponseHandler(self, requestId: int, callback: Callable[[dict], Any], ackPermitted: bool=False): | ||
self.responseHandlers[requestId] = ResponseHandler(callback=callback, ackPermitted=ackPermitted) | ||
|
||
def _sendPacket(self, meshPacket: mesh_pb2.MeshPacket, destinationId: Union[int,str]=BROADCAST_ADDR, wantAck: bool=False): | ||
"""Send a MeshPacket to the specified node (or if unspecified, broadcast). | ||
|
@@ -1129,16 +1134,18 @@ def _handlePacketFromRadio(self, meshPacket, hack=False): | |
requestId = decoded.get("requestId") | ||
if requestId is not None: | ||
logging.debug(f"Got a response for requestId {requestId}") | ||
# We ignore ACK packets, but send NAKs and data responses to the handlers | ||
# We ignore ACK packets unless the callback is named `onAckNak` | ||
# or the handler is set as ackPermitted, but send NAKs and | ||
# other, data-containing responses to the handlers | ||
routing = decoded.get("routing") | ||
isAck = routing is not None and ("errorReason" not in routing or routing["errorReason"] == "NONE") | ||
if not isAck: | ||
# we keep the responseHandler in dict until we get a non ack | ||
handler = self.responseHandlers.pop(requestId, None) | ||
if handler is not None: | ||
if not isAck or (isAck and handler.__name__ == "onAckNak"): | ||
logging.debug(f"Calling response handler for requestId {requestId}") | ||
handler.callback(asDict) | ||
# we keep the responseHandler in dict until we actually call it | ||
handler = self.responseHandlers.get(requestId, None) | ||
if handler is not None: | ||
if (not isAck) or handler.callback.__name__ == "onAckNak" or handler.ackPermitted: | ||
handler = self.responseHandlers.pop(requestId, None) | ||
logging.debug(f"Calling response handler for requestId {requestId}") | ||
handler.callback(asDict) | ||
Comment on lines
+1142
to
+1148
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (this is the change that fixes the bug -- previously we were not checking for |
||
|
||
logging.debug(f"Publishing {topic}: packet={stripnl(asDict)} ") | ||
publishingThread.queueWork( | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now this is only added to sendData, but I will try to go through and add it to other helpers too, for library users' sake.