-
Notifications
You must be signed in to change notification settings - Fork 44
Transport: add support for scratchpad as chunk #314
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
base: master
Are you sure you want to change the base?
Conversation
| if acc is not None: | ||
| if (acc["seq"] != request.seq | ||
| or acc["total_size"] != total_size | ||
| or offset == 0): |
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.
visually indented line with same indent as next logical line
| "--gateway_max_scratchpad_size", | ||
| type=self.str2int, | ||
| default=os.environ.get("WM_GW_MAX_SCRAT_SIZE", None), | ||
| help=("Maximum scratchpad size a gateway can accept. If scratchpad is bigger" |
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.
line too long (89 > 79 characters)
| logging.info("Scratchpad fully received for %s", request.sink_id) | ||
|
|
||
| try: | ||
| res = sink.upload_scratchpad(request.seq, acc["buffer"]) |
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.
What happens with the below _send_otap_response call if this throws an exception? Should the res be set to some error before this try block?
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.
On the other hand, it doesn't seem that it might throw an exception, and other call is without a try. Do you remember why you added this inside a try block?
| type=self.str2int, | ||
| default=os.environ.get("WM_GW_MAX_SCRAT_SIZE", None), | ||
| help=("Maximum scratchpad size a gateway can accept. If scratchpad is bigger" | ||
| "it must be sent as chunks smaller or equal to this value"), |
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.
This is consistent with the backend api naming and documentation, but should it be clarified more that it's the max chunk size and it's only a "suggestion" for the backend (since there is no limit in the code for max chunk size)?
Feature can be tested with Console on this branch:
https://wirepas-tools.github.io/mqtt-console/chunk/