Skip to content
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

add check command to POST commands #772

Merged
merged 8 commits into from
Oct 13, 2023
Merged

Conversation

mkmer
Copy link
Contributor

@mkmer mkmer commented Sep 15, 2023

Description:

While researching the event API, I discovered the Android app polls the command status to determine when to get the results. This improves the performance quite a bit as we don't have to wait for the last backoff (some times 24 seconds).

All POST methods receive a response code with the command id and network id. Polling at 1 second (same as the app) until it receives a complete.
If any of the keys are missing, the API will fall back to the previous retry method.

Related issue (if applicable): fixes #

Checklist:

  • Local tests with tox run successfully PR cannot be meged unless tests pass
  • Changes tested locally to ensure platform still works as intended
  • Tests added to verify new code works

@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

Merging #772 (69c6608) into dev (93ce7c3) will increase coverage by 0.01%.
Report is 1 commits behind head on dev.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev     #772      +/-   ##
==========================================
+ Coverage   99.64%   99.65%   +0.01%     
==========================================
  Files           8        8              
  Lines        1404     1447      +43     
==========================================
+ Hits         1399     1442      +43     
  Misses          5        5              
Flag Coverage Δ
unittests 99.65% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
blinkpy/api.py 100.00% <100.00%> (ø)
blinkpy/camera.py 98.43% <100.00%> (+0.05%) ⬆️
blinkpy/sync_module.py 100.00% <100.00%> (ø)

@mkmer mkmer changed the title add check command to PUT add check command to POST commands Sep 16, 2023
blinkpy/api.py Outdated Show resolved Hide resolved
@mkmer
Copy link
Contributor Author

mkmer commented Sep 18, 2023 via email

@mkmer
Copy link
Contributor Author

mkmer commented Sep 18, 2023

For documentation: Command polled message looks like this:
"waiting"
{'complete': False, 'status': 0, 'status_msg': 'Command succeeded', 'status_code': 908, 'commands': [{'id': 701635684, 'created_at': '2023-09-18T14:56:20+00:00', 'updated_at': '2023-09-18T14:56:20+00:00', 'deleted_at': None, 'execute_time': '2023-09-18T14:56:20+00:00', 'command': 'local_storage_media', 'state_stage': 'rest', 'stage_rest': '2023-09-18T14:56:20+00:00', 'stage_cs_db': None, 'stage_cs_sent': None, 'stage_sm': None, 'stage_dev': None, 'stage_is': None, 'stage_lv': None, 'stage_vs': None, 'state_condition': 'new', 'sm_ack': None, 'lfr_ack': None, 'sequence': None, 'attempts': 0, 'transaction': 'v99BE8KyESywBris', 'player_transaction': '-zaSpLUHFb_th-h9', 'server': None, 'duration': None, 'by_whom': 'Computer - ', 'diagnostic': False, 'debug': '', 'opts_1': 0, 'target': 'sync_module', 'target_id': 560254, 'trace_parent': None, 'sync_module_id': 560254, 'parent_command_id': None, 'camera_id': None, 'siren_id': None, 'firmware_id': None, 'network_id': 411028, 'account_id': 484574}], 'media_id': None}

"complete"
{'complete': True, 'status': 0, 'status_msg': 'Command succeeded', 'status_code': 908, 'commands': [{'id': 701636621, 'created_at': '2023-09-18T14:57:19+00:00', 'updated_at': '2023-09-18T14:57:22+00:00', 'deleted_at': None, 'execute_time': '2023-09-18T14:57:19+00:00', 'command': 'local_storage_media', 'state_stage': 'sm', 'stage_rest': '2023-09-18T14:57:19+00:00', 'stage_cs_db': '2023-09-18T14:57:19+00:00', 'stage_cs_sent': '2023-09-18T14:57:19+00:00', 'stage_sm': '2023-09-18T14:57:20+00:00', 'stage_dev': None, 'stage_is': None, 'stage_lv': None, 'stage_vs': None, 'state_condition': 'done', 'sm_ack': 1, 'lfr_ack': None, 'sequence': 12481, 'attempts': 0, 'transaction': 'ZHrCoiLMB4ky9rbX', 'player_transaction': 'HsVnjuOFT_SLIIqk', 'server': None, 'duration': None, 'by_whom': 'Computer - ', 'diagnostic': False, 'debug': '', 'opts_1': 0, 'target': 'sync_module', 'target_id': 560254, 'trace_parent': None, 'sync_module_id': 560254, 'parent_command_id': None, 'camera_id': None, 'siren_id': None, 'firmware_id': None, 'network_id': 411028, 'account_id': 484574}], 'media_id': None}

@mkmer
Copy link
Contributor Author

mkmer commented Sep 19, 2023

Looking at this a bit further, If the status_code is not 908, we want to return False, even if the command is complete as an indicator (in the future anyway) that the command is complete but failed. Just moved the if tests around to achieve it.

@fronzbot
Copy link
Owner

Oh completely forgot about this- I think my comments in the other PR relate to this one actually.

@fronzbot fronzbot mentioned this pull request Oct 11, 2023
3 tasks
@mkmer
Copy link
Contributor Author

mkmer commented Oct 11, 2023

I didn't realize you had a question/suggestion in there :) I agree it makes sense to NOT be stuck if the API behaves poorly.
Maybe we should raise an error or something if it times out? For now it will just fall back to the old behavior of trying again

@fronzbot
Copy link
Owner

Yeah adding an error message or something would make sense at some point, but for now what's there is fine.

Can you just remove the .vscode folder check-in (maybe worth adding to .gitignore as well)? I'll merge after that 👍

@mkmer
Copy link
Contributor Author

mkmer commented Oct 11, 2023

Now I'm getting things mixed up :) Should be good now.
Hopefully we can get the HA guys to move on that PR....

@mkmer
Copy link
Contributor Author

mkmer commented Oct 13, 2023

Maybe we could do a release on pypi and I'll update the PR over at HA to use it - since it appears to be "stuck", might as well bump it one more time :)

@fronzbot fronzbot merged commit 12b6ced into fronzbot:dev Oct 13, 2023
8 checks passed
@fronzbot
Copy link
Owner

Yeah good idea, I'll do that shortly

@fronzbot fronzbot deleted the command_check branch October 13, 2023 15:03
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