-
-
Notifications
You must be signed in to change notification settings - Fork 286
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 Remote Command Over WiFi #255
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, more network stuff for me to refactor in impl/feature-retrieval
LGTM
It'd be cool if you could rebase this onto #256 |
pain^2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, would be nice if it were rebased against the network rewrite stuff tho
e3d2e80
to
758f8a1
Compare
IP check is added, so that remote command would only accept connection from the IP of connected SlimeVR Server. When the server is not connected, any IP could connect to the remote command, but will not receive any response, and only accept the |
commit history is a little broken on this and #262 😓 |
We squash it so it doesn't matter.. |
Rebased to main after v0.4.0 and tested on ESP32-S3, remote commands and server IP checks are looking good. |
@@ -39,6 +39,7 @@ namespace SerialCommands { | |||
CmdCallback<6> cmdCallbacks; | |||
CmdParser cmdParser; | |||
CmdBuffer<256> cmdBuffer; | |||
bool cmdFromRemote = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bool cmdFromRemote = false; | |
bool cmdFromRemote = false; |
@@ -73,8 +74,8 @@ class FirmwareFeatures { | |||
public: | |||
enum EFirmwareFeatureFlags: uint32_t { | |||
// EXAMPLE_FEATURE, | |||
REMOTE_COMMAND = 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this really be 0? Is there a reason we started at 1?
#if USE_REMOTE_COMMAND && ALLOW_REMOTE_WIFI_PROV | ||
if (cmdFromRemote && !networkConnection.isConnected()) return; | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is used multiple times throughout this file, but it's not applied to every command? Why not just put it at the parser before the command functions are called? Also why is it on get and such? Isn't WiFi prov just for setting the WiFi?
This PR add a remote command server running in parallel with the serial command, sharing the same command buffer, command lists, and logger outputs. All the data printed through
Logger
will be forked to the remote when connected, while the ones usingSerial
won't be forked.The remote command server could be accessed through telnet clients. Would be easy to integrate into server since it use a simple TCP socket to transmit the same data as serial commands do.
Currently this remote command mostly benefits for checking TCAL status remotely, as other commands are mostly for debugging instead of functions. This could be used for future function commands which are too minor to occupy a packet type or doesn't fit well in a UDP packet like largely varying data length.