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

Firmware protocol #37

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from
Draft

Firmware protocol #37

wants to merge 15 commits into from

Conversation

loucass003
Copy link
Member

No description provided.

@TheButlah TheButlah added the Status: Help Wanted Needs some help to make progress label Oct 5, 2022
@TheButlah TheButlah added the Area: Hardware Protocol Related to communication with hardware/software trackers label Nov 14, 2022
@TheButlah
Copy link
Collaborator

TheButlah commented Jan 7, 2023

So it doesn't get lost, here was my review of this draft:
https://discord.com/channels/817184208525983775/1006626234685460651/1059246303105781811

some initial feedback on the firmware protocol:

  1. We want to support really small packet sizes ideally without fragmenting anything. This means that vectors inside flatbuffers is generally a nono. Instead of for example having a table of tracker info and then a vector of sensor infos, use separate packets for sensor info and tracker info. We can send a separate sensor info packet for every sensor
  2. I see many (required) fields. Its a breaking change to make these fields optional, such as when we realize we need to change the schema and remove a field or change it. Please remove all (required) tags
  3. I was on the fence whether the server should be the multicast broadcaster (implying you will always have to broadcast even when all your trackers are already connected) vs the tracker being the broadcaster. It looks like rn the server is the broadcaster but I'm not sure I'm a fan of this. Furthermore it means the trackers will receive broadcasts from the server and have to waste RX power and cycles on processing that
  4. Part of the DeviceSensorInfo should indicate whether the imu data is fused or not.
  5. The protocol provides no way to provide positional data. This is a problem, it can be addressed perhaps be renaming ImuMovement to something more generic, or introducing a new packet type for positional tracked devices. I'm somewhat more inclined towards the latter, tbh. If you go with the former, ImuMovement should probably be renamed to OrientationData or something
  6. there are also a lot of primitives that are not marked as optional, which means they default to 0. I'm not sure this is a good thing, assigning them to a default of null is probably better. For example, rssi is not necessarily something all firmware will know, so if it doesn't know it it will send 0. But the server then has no way to differentiate between a zero value and the actual rssi
  7. In line with keeping tables small, SensorInfo and TrackerInfo should not be in PairingInfo. Paring is orthogonal to information about the hardware. Pairing should only have the absolute bare minimum needed to establish the tracker and/or server address/identity
  8. Current design provides no way to multiplex on a single "connection". For example, this protocol should also be able to be used over usb serial, with a dongle connected to several nrf devices. This means the dongle is going to need to multiplex the flatbuffers for several devices across the same usb serial connection. I think the right way to do that is to have an optional unique ID (could be the mac address) as part of the header table.
  9. In keeping with the issues I have with (require), we are currently forced to send a mac address along with every single message sent to the server. Why? Its extremely wasteful and redundant to do that. It should be optional.
  10. we should not call the payload req_rep when we are not actually doing a req/rep pattern. just call it payload

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Hardware Protocol Related to communication with hardware/software trackers Status: Help Wanted Needs some help to make progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants