Skip to content

Move Wi-Fi code to strategy pattern#534

Open
gorbit99 wants to merge 6 commits into
mainfrom
refactor/move-comms-to-strategy
Open

Move Wi-Fi code to strategy pattern#534
gorbit99 wants to merge 6 commits into
mainfrom
refactor/move-comms-to-strategy

Conversation

@gorbit99

Copy link
Copy Markdown
Contributor

This PR does most of the legwork to decouple the Wi-Fi implementation from the rest of the codebase. This should allow for easier implementation of alternative connection/communication methods, like ESP-Now.

Two major refactors were done. First, the Wi-Fi handler code was remade using the state pattern, which leads to a slightly cleaner implementation. Secondly the Wi-Fi code was encapsulated into a strategy pattern implementation.

In theory creating a new implementation of the CommunicationsStrategy interface, alongside the appropriate code for handling that new strategy in the network manager class should allow for alternative communication methods. It's likely that further changes will need to be done to the interface for other classes to work as expected, but I tried making it as basic as possible.

This PR should not change any functionality in a significant way.

@gorbit99
gorbit99 requested a review from Eirenliel as a code owner July 15, 2026 07:02
@gorbit99
gorbit99 requested review from Eirenliel and unlogisch04 and removed request for Eirenliel July 15, 2026 07:52
@unlogisch04

Copy link
Copy Markdown
Contributor

After a quit short look at the design/flow:
CommunicationStrategy -> that class is more like the base virtual for the different drivers. (i think the name is not fitting here)

I'm not sure if we want to keep that layout of the direct call to network functions from the sensor class. It would be probably better to implement a buffer type handling of the sending/receiving data.
This would allow the network communication more independent from the IMU loop. (as example if the network only operates at 50 tps stable, we don't want a other buffer to fill up with late data, we can't delete)

For receiving would also make a buffer, and nack (not sure if that is currently possible in the feature flags) when it is full. The buffer allows to handle the commands at a given time. And not without delay interrupting stuff.

We also need to care about disconnect/changing protocol. I see there a high likely hood of getting nullptr references. (.comms() = nullptr for a short of time)

So what i think would be better is the following:
sensor -> (sends data to buffer) databroker <- network asks for the latest data
databroker handles mutex and thread safety,.. and discarding of old data. (maybe a second mode that stores all of them with time stamp and sends them when available for a better mocap recreation)

The current implementation assumes the network is all the time faster than the IMU preparing data. But with ESP-Now I'm not sure if this always is the case. Well also seems also not always the case with WiFi.

@gorbit99

Copy link
Copy Markdown
Contributor Author

CommunicationStrategy -> that class is more like the base virtual for the different drivers. (i think the name is not fitting here)

It's the strategy pattern (https://refactoring.guru/design-patterns/strategy)

I'm not sure if we want to keep that layout of the direct call to network functions from the sensor class. It would be probably better to implement a buffer type handling of the sending/receiving data.

The beauty of this is that it doesn't matter much. You can implement a buffering strategy that gets the data from the tracker, buffers it up (or more realistically, only keeps the latest information) and either hands that over to the corresponding strategy or those access that instead. Though I'm doubtful that either wifi or espnow would be too slow to handle our workload.

For receiving would also make a buffer, and nack (not sure if that is currently possible in the feature flags) when it is full. The buffer allows to handle the commands at a given time. And not without delay interrupting stuff.

We barely receive anything, I don't think there's a point. We would also need to manually NACK, since this is UDP.

We also need to care about disconnect/changing protocol. I see there a high likely hood of getting nullptr references. (.comms() = nullptr for a short of time)

I don't see a reason to ever set that to a nullptr. If a disconnection happens, the strategy is kept around to reconnect, see:
https://github.com/SlimeVR/SlimeVR-Tracker-ESP/pull/534/changes#diff-f50b05d6703721416f51842e75dc5bb1374fee121579f3e4aeb896f49ce16f19R73
With a protocol change, you also just immediately swap.

@unlogisch04

unlogisch04 commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

It's the strategy pattern (https://refactoring.guru/design-patterns/strategy)

ok, still not fan of it, but ok.

The beauty of this is that it doesn't matter much. You can implement a buffering strategy that gets the data from the tracker, buffers it up (or more realistically, only keeps the latest information) and either hands that over to the corresponding strategy or those access that instead. Though I'm doubtful that either wifi or espnow would be too slow to handle our workload.

I agree, but i also hope that it will be easier to handle packetbundle stuff that way, even if we don't gather all sensor info in 1 loop. Currently we send the packets into a udp buffer that will be sent out anytime. I would love to be able to have a little control over that. Not sure if we can on esp8266_arduino maybe i have to dig deeper there.

We barely receive anything, I don't think there's a point. We would also need to manually NACK, since this is UDP.

hm maybe it does not matter. but when then i would add the same as sending

I don't see a reason to ever set that to a nullptr. If a disconnection happens, the strategy is kept around to reconnect, see:
https://github.com/SlimeVR/SlimeVR-Tracker-ESP/pull/534/changes#diff-f50b05d6703721416f51842e75dc5bb1374fee121579f3e4aeb896f49ce16f19R73
With a protocol change, you also just immediately swap.

hm ok no problem with multicore?

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