Add mode switching without reflash#88
Conversation
lorow
left a comment
lorow
left a comment
There was a problem hiding this comment.
That's a lot of good work, much appreciated. Leaving mostly some questions, nothing major :D
Once more, thanks for taking care of this!
| deviceModeManager->setMode(DeviceMode::WIFI_MODE); | ||
| log_i("[CommandManager] Switching to WiFi mode after receiving credentials"); | ||
|
|
||
| OpenIrisTasks::ScheduleRestart(2000); |
There was a problem hiding this comment.
This might be a bad idea, we're sending two commands in one payload in the flasher, first one to setup wifi and second to setup mDNS, so the second one might fail or won't get executed at all since they're executed in the order they were received.
I think it'd be better to introduce one more command - RESTART_DEVICE and send it as the very last command so that we're sure we've got everything set properly before
There was a problem hiding this comment.
Yep! That makes sense; I'll add it then! i wasnt really sure how the flasher worked, but that makes sense
|
|
||
| const CommandType CommandManager::getCommandType(JsonVariant& command) { | ||
| if (!command.containsKey("command")) | ||
| if (!command["command"].is<const char*>()) |
There was a problem hiding this comment.
Just wondering, why the change? Is .is<T> is faster / more memory efficient than .containsKey()?
There was a problem hiding this comment.
command["command"].is<const char*>() it's the recommended approach in ArduinoJson because containsKey() is deprecated lol and i didn't like the warnings
| deviceModeManager->setMode(DeviceMode::USB_MODE); | ||
| log_i("[CommandManager] Switching to USB mode after wiping credentials"); | ||
|
|
||
| OpenIrisTasks::ScheduleRestart(2000); |
There was a problem hiding this comment.
Something happened here with formatting or github crewed up?
There was a problem hiding this comment.
Ah! i wrote some code that didn't work, deleted it and then forgot to fix the restart fomat imao
| } | ||
|
|
||
| void DeviceModeManager::init() { | ||
| preferences.begin(PREF_NAMESPACE, false); |
There was a problem hiding this comment.
I'm not sure if I'm sold on this. We've got a config manager that takes care of manipulating everything related to preferences and configuration so we could move most of the responsibility into it, what do ya think?
Granted, the idea of DeviceModeManager being a singleton would have to go and with it a bit of the implementation, but might be a bit cleaner?
There was a problem hiding this comment.
perchance yeah, I needed some lightweight code to do what i wanted at the time, so ill change it to use config manager lol
| } | ||
|
|
||
| DeviceModeManager* deviceModeManager = DeviceModeManager::getInstance(); | ||
| if (deviceModeManager) { |
There was a problem hiding this comment.
from what I gather, getInstance() will return the already existing instance or create a fresh one, do we still have to check for a nullpointer then?
| } else { | ||
| CommandsPayload commands = {doc}; | ||
| this->commandManager->handleCommands(commands); |
| DeviceMode currentMode = DeviceModeManager::getInstance()->getMode(); | ||
| if (currentMode == DeviceMode::USB_MODE) { |
There was a problem hiding this comment.
how about:
if (DeviceModeManager::getInstance()->getMode() == DeviceMode::USB_MODE)
I'm pretty sure the compiler will optimize it away anyway, but we can make it slightly cleaner
| if (Serial.available()) { | ||
| yield(); | ||
| } |
There was a problem hiding this comment.
So, the way I understand it. We're connecting to wifi, but then a command came telling us to use usb instead, so we've changed the mode. Why do we have to yield here then? Shouldn't wifi manager just stop in its track once we disconnect?
ESP/src/main.cpp
Outdated
| if (deviceModeManager && deviceModeManager->getMode() == DeviceMode::USB_MODE) { | ||
| log_i("[SETUP]: Mode changed to USB before network initialization, aborting"); | ||
| WiFi.disconnect(true); | ||
| return; | ||
| } |
There was a problem hiding this comment.
We're doing the same thing three times, I'd move it out to a smaller helper function, a bit easier to maintain later
| def send_command(self, command_obj): | ||
| if not self.serial_conn: | ||
| print("Serial connection not established") | ||
| return False |
There was a problem hiding this comment.
we're returning here a False, a Json object and a None. I'd replace the False with a None just to be consistent
|
Also, builds are failing because (Like Assassin mentioned on discord) |
Removed DeviceModeManager and integrated device mode handling into ProjectConfig for better maintainability. Added RESTART_DEVICE command for explicit device reboots and updated related tests to verify reboot functionality. Simplified mode checks across the codebase by using ProjectConfig directly.
2 participants
This version unified wifi and USB firmware, making it so now setup can be done on the actual application in real deployment.