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 more compact bundle protocol #999

Merged
merged 7 commits into from
Apr 28, 2024
Merged

Conversation

9il
Copy link
Contributor

@9il 9il commented Apr 21, 2024

@ImUrX I think we could adopt SlimeVR protocol instead of introducing our own. But we would need to add some features.

There is a higher chance that a smaller UDP will be delivered than a larger one. Also, we need to reduce the overall amount of data we send because iOS can kill our application if it consumes too many resources (battery, network, CPU, etc.).

Please, if possible, add #997 and #998 with this MR into the upcoming release v0.12.

@9il 9il force-pushed the BUNDLE_V2 branch 2 times, most recently from 94a2f9b to 24b8568 Compare April 21, 2024 11:37
@ImUrX
Copy link
Member

ImUrX commented Apr 21, 2024

Bundle protocol exists as a bypass for old hardware that doesn't implement frame aggregation in their networking implementation (ESP8266).
I can see that the optimization of this implementation is only having a subset of packets being supported (as maximum package type is 255u and length is also 255u). But there is no firmware development which verifies that this actually is useful by some benchmarks and a mobile device wouldn't really be taking advantage of it because it already does via frame aggregation

@9il
Copy link
Contributor Author

9il commented Apr 21, 2024

@ImUrX What is the frame aggregation you are talking about?

@ImUrX
Copy link
Member

ImUrX commented Apr 21, 2024

It's used in Wi-Fi protocols https://en.wikipedia.org/wiki/Frame_aggregation
Bundling was implemented in SlimeVR as ESP8266 doesn't support frame aggregation.

@9il
Copy link
Contributor Author

9il commented Apr 21, 2024

OK, I see. However, the main point is to reduce the amount of data transferred over the air. This PR reduces it more than three times.

That isn't beneficial for slimes because they have relatively large batteries and don't need to aggregate data from dozens of sensors.

On the other hand, the volume of data transmitted over the air is a critical factor for mobile phones, primarily due to power consumption concerns. For instance, iOS has a policy to terminate any background app that consumes some energy. Therefore, if an iPhone is required to act as a proxy, the data transfer needs to be carefully managed.

The same thing happens for some Android vendors as well.

Mocopi example can't be used here because they have only six sensors and a lower frame rate.

@ImUrX
Copy link
Member

ImUrX commented Apr 21, 2024

Please split your PR into two, having the IMU packet in another PR and the simplified bundle protocol in another then.

@9il 9il changed the title Add more compact bundle protocol and IMU frame (rot+acc) Add more compact bundle protocol Apr 21, 2024
@9il
Copy link
Contributor Author

9il commented Apr 21, 2024

@ImUrX The new packed is here #1001 now

@ImUrX ImUrX requested a review from 0forks April 21, 2024 16:08
@ImUrX ImUrX added Area: Hardware Protocol Related to communication with hardware/software trackers Type: Enhancement Adds or improves a feature Area: Server Related to the server labels Apr 21, 2024
@9il 9il requested a review from 0forks April 23, 2024 10:53
@9il
Copy link
Contributor Author

9il commented Apr 23, 2024

OK, CI passes, please merge it :)

@ImUrX
Copy link
Member

ImUrX commented Apr 23, 2024

Sorry but we are releasing 0.12 without this PR, we have a release candidate system and it's been almost a month since we released the first one.

We don't really want to do another release candidate because of this.

@ImUrX
Copy link
Member

ImUrX commented Apr 27, 2024

Can you update the PR?

@9il
Copy link
Contributor Author

9il commented Apr 27, 2024

Do you mean an update from the main? Done.

@ImUrX ImUrX merged commit 6d01491 into SlimeVR:main Apr 28, 2024
8 checks passed
@9il 9il deleted the BUNDLE_V2 branch April 28, 2024 19:53
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 Area: Server Related to the server Type: Enhancement Adds or improves a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants