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

trackerNum should not exists inside the protocol #100

Open
loucass003 opened this issue Feb 18, 2023 · 5 comments
Open

trackerNum should not exists inside the protocol #100

loucass003 opened this issue Feb 18, 2023 · 5 comments
Labels
Area: Application Protocol Related to communication with apps like the GUI, overlay, games Type: Discussion Further information is requested

Comments

@loucass003
Copy link
Member

loucass003 commented Feb 18, 2023

currently the tracker id holds the device id and the tracker num. like so

table TrackerId {
    /// The device the tracker is associated with. If there is no hardware device it is
    /// associated with, this should be `null`.
    device_id: solarxr_protocol.datatypes.DeviceId;
    /// There are possibly multiple trackers per device. This identifies which one.
    tracker_num: uint8;
}

but the issue with this is that you need to know the device id to change anything with the tracker. In many instance in the server code you have to do a lot of checks to make sure you are getting the right tracker.

also 99% of the time you are not changing settings to a device but to the tracker. So working with the device as the id is not that necessary.
And when you are mutating a device you will only send the device id anyway, no need to send a trackerNum.

here in the server code a function to get the tracker from its tracker id

	public Tracker getTrackerById(TrackerIdT id) {
		for (Tracker tracker : trackers) {
			if (tracker.getTrackerNum() != id.getTrackerNum()) {
				continue;
			}

			// Handle synthetic devices
			if (id.getDeviceId() == null && tracker.getDevice() == null) {
				return tracker;
			}

			if (
				tracker.getDevice() != null
					&& id.getDeviceId() != null
					&& id.getDeviceId().getId() == tracker.getDevice().getId()
			) {
				// This is a physical tracker, and both device id and the
				// tracker num match
				return tracker;
			}
		}
		return null;
	}

the protocol should hold unique ids for the device and the tracker.
lets see what a rewrite would look like

table TrackerId {
    id: uint8;
}


table TrackerData {
    tracker_id: solarxr_protocol.datatypes.TrackerId;
   // i removed everything below for convenience because it does not add any value to that topic
}

struct DeviceId {
    id: uint8;
}

table DeviceData {
    id: solarxr_protocol.datatypes.DeviceId;
    // i removed everything below for convenience because it does not add any value to that topic
}

@loucass003 loucass003 added Type: Discussion Further information is requested Status: Unlabeled A maintainer has not yet labeled this Area: Application Protocol Related to communication with apps like the GUI, overlay, games and removed Status: Unlabeled A maintainer has not yet labeled this labels Feb 18, 2023
@TheButlah
Copy link
Collaborator

TheButlah commented Feb 19, 2023

I don't mind having tracker id as its own unique id. I believe this design was because originally we thought this protocol would also be sent from the firmware itself, and the firmware can't easily make a unique id for a tracker. It can only make a unique id for the device.

But that limitation of firmware is irrelevant to the application protocol that we have in SolarXR. So feel free to make the change.

@Eirenliel
Copy link
Member

it could still be used for firmware protocol, but tracker_id on one side won't match tracker_id on the other, which is fine

@loucass003
Copy link
Member Author

the firmware protocol wont be in that section of the schema. it will be its own schema anyway.
And from the looks of it there probably wont be a flatbuffers firmware protocol because of the footprint of the flatbuffers library

@TheButlah
Copy link
Collaborator

footprint of the flatbuffers library

footprint of the payload (not the library). But otherwise, yeah I agree

@Eirenliel
Copy link
Member

that's not the reason to make different structures, a recepie for confusion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Application Protocol Related to communication with apps like the GUI, overlay, games Type: Discussion Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants