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

Cleanup of airctrl.py #81

Merged
merged 11 commits into from
Apr 16, 2021
Merged

Cleanup of airctrl.py #81

merged 11 commits into from
Apr 16, 2021

Conversation

Cyber1000
Copy link
Contributor

tl;dr

I did a bigger refactoring last year and added the coap-part, but changes in airctrl.py where only a first approach.
There where differences in output and many redundances between the 3 protocols (http, plaincoap and coap)
This PR cleans airctrl.py - output will be the same no matter which protocol you use

longer information, points to pay attention

changes to cli:

  • added CliBase to have a common formatting class depending on STATUS_TRANSFORMER

  • updated STATUS_TRANSFORMER class:

    • some values where missing here (for example "Gas" in ddp)
    • added an error value I found a while ago -> 193: 'F0 (pre-filter) must be cleaned'
    • added an additional index to define sets like "filter", "firmware" (used for easier output with get_firmware, get_filters)
  • attention - changed formatting:

    • may change ordering of output
      • for now it will be ordered like it comes out of the device
      • I think it will be possible to take the ordering of STATUS_TRANSFORMER, if someone wishes this
    • may add additonal spaces between key and value
    • minor changes (plain coaps` output was "Software version:", the other protocols outputted "Version:")
    • Ordering, spacing was different between coap, plaincoap and http, so I think it is better to have the same output on all protocols. Coap doesn't change at all, the other 2 may have this issue.
    • see also testdata (data.json)
  • attention - changed behaviour:

    • http-protocol was the only one which executed get_values immediately after set_values, the other protocols didn't output anything after calling set_values
    • So I removed it from Class HTTPAirCli (and removed a test in test_http.py)
    • If this is not what we want, we may:
      • add get_values back again to HTTPAirCli.set_values
      • add get_values to CliBase.set_values (and therefore for all protocols)
    • Not sure what's best here
  • tests were adjusted and build green

@rgerganov
Copy link
Owner

Thank you for the PR. I will try to review and provide feedback soon.

@rgerganov
Copy link
Owner

Sorry for taking me so long to get to this. I have reviewed and tested your changes, everything looks fine to me!
About the behavior change for HTTP devices -- I prefer to get the updated values after changing settings but I am also fine with your approach. I think the best way to switch between those two behaviors is at the end of the main function:

        if values:
            c.set_values(values, debug=args.debug)
        else:
            c.get_status(debug=args.debug)

vs.

        if values:
            c.set_values(values, debug=args.debug)
        c.get_status(debug=args.debug)

@rgerganov rgerganov merged commit cf932c7 into rgerganov:master Apr 16, 2021
@Cyber1000
Copy link
Contributor Author

No problem, was busy too. Well yes I think the proposed change at the end of the main function would be the best position if we want to add the get after a set. Maybe in another PR.
Thanks for merging!

@Cyber1000 Cyber1000 deleted the cleanup branch April 16, 2021 23:48
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