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

For discussion: Extended commands #112

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Smeat
Copy link
Collaborator

@Smeat Smeat commented Apr 3, 2020

This adds a skeleton for sending extended commands over the normal connections.

This way we'll be able to completely control everything using the normal connections and will replace the json stuff from the webserver in the future and use websockets for the communication. This allows us to make the page much more responsive (no reloading required).
The json way also consumes a large amount of dynamic memory, which should be avoided.

Still open for suggestions on how to improve this.

Test for compatability:

  • Android app
  • ios app
  • Racegod
  • Livetime

#define EXTENDED_CALIB_MAX 'C'
#define EXTENDED_CALIB_START 's'
#define EXTENDED_CALIB_STATUS 'S'
#define EXTENDED_VOLTAGE_TYPE 'v'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably needs a better name to make it more clear what it's for.

#define EXTENDED_WIFI_CHANNEL 'W' // half byte
#define EXTENDED_WIFI_PROTOCOL 'w' // half byte
#define EXTENDED_FILTER_CUTOFF 'F'
#define EXTENDED_MULTIPLEX_OFF 'm'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. A more descriptive name would be nice

#define EXTENDED_WIFI_PROTOCOL 'w' // half byte
#define EXTENDED_FILTER_CUTOFF 'F'
#define EXTENDED_MULTIPLEX_OFF 'm'
#define EXTENDED_UPDATE_PROGRESS 'U'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this since it's currently unused. Could always be added back at a later point if a need for it arises.

@swampmonster
Copy link
Contributor

swampmonster commented Apr 4, 2020

I'm thinking it might be a good idea to be able to mark a client as supporting the extended format.
Like say when a client connects it can send along a command to indicate that E is supported that way we can avoid sending a lot of excess data to clients that can't do anything with it.

I think this would only apply in a scenario where there are more than one clients connected though because i haven't seen any code that doles out data on a per client basis it goes to everyone all the time if more than one is connected. I could however be mistaken

@Smeat
Copy link
Collaborator Author

Smeat commented Apr 4, 2020

I added a short description on what these messages might do. I am using them in my fork, but just see them as a suggestion we might implement later.

Yeah at first only one client was supported on the protocol. When I added support for multiple clients I just made everything a broadcast.
But as most messages are only sent upon request having a few extra messages for an unsupported client shouldn't be that bad.

@swampmonster
Copy link
Contributor

swampmonster commented Apr 4, 2020

I added a short description on what these messages might do. I am using them in my fork, but just see them as a suggestion we might implement later.

Yeah at first only one client was supported on the protocol. When I added support for multiple clients I just made everything a broadcast.
But as most messages are only sent upon request having a few extra messages for an unsupported client shouldn't be that bad.

You're right it's not that much extra data. I would like to not broadcast everything but that's for a different pull request :)

Edit:
A concern might be what clients that don't support E does however. If they handle an unkown command gracefully

Copy link
Contributor

@swampmonster swampmonster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@Smeat
Copy link
Collaborator Author

Smeat commented Apr 4, 2020

If the implementation is correct, these messages should just be discarded. Not sure how you'd implement this wrong.
This is also only a concern if you have >= 2 clients where one uses the extended commands.
But this should definitely be tested before merging.

No problem when using the Android app.

@swampmonster
Copy link
Contributor

If the implementation is correct, these messages should just be discarded. Not sure how you'd implement this wrong.
This is also only a concern if you have >= 2 clients where one uses the extended commands.
But this should definitely be tested before merging.

No problem when using the Android app.

I’ll test it in the iOS app

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