-
Notifications
You must be signed in to change notification settings - Fork 28
Mode separation #25
Mode separation #25
Conversation
Start reorganizing the code a bit to be able to build a more plugin like API for the different modes
The equal signatures make it easier to modularize the modes.
Since all mode functions are now using the same signature, we can now use a more systematic approach for switching the modes
These classes contain the render functions for the moment, but may be used to encapsulate all mode specific things in the future.
The processing of the JSONified mode specific config settings can now be found in the mode classes where it belongs.
94a20a5
to
7c28557
Compare
7c28557
to
74a3834
Compare
Awesome stuff. This may take me a while to read through. I also want to give it a test as well and hopefully make some additions. |
virtual void applyConfig(JsonVariant& settings); | ||
}; | ||
|
||
std::map<String, ModeBase*> modes; |
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.
More just thinking out loud here: I like this a lot. I think map will be best container option currently. Only thoughts are if it were to become a dynamic list where you could add and remove modes would it be better as a list?
I cannot commend you enough for your work on this. I thought I was going to spend a day getting myself up to speed but since you wrote this so well I was able to do it in one night. The code is fantastic, and I think it should really start pushing towards a library since its using classes now. I did find and fix one little bug where during a mode change the lights would change to that mode, dim down, then dim up rather than dim down, change mode, then dim up. Luckily this was already written and just required a change of variable in one spot. I also hopefully reduce the potential of a crash by moving this logic to an else block as you will see. Once again, amazing work, very happily merging this in. Looking forward to whats next (probably #23). |
Could you write a short documentation on how you would add new modes and what one needs to consider in this regard (e.g., how do I nicely integrate them into the web UI)? That would be great. :) |
With this merge and a the new branch I created I want to change a few things to make it easier to add modes. If you look at the "new modes" branch you can start to see the changes I'm making to get to this. |
Thanks! Looks interesting. Next I would like to split up the website and move the mode specific things to the mode classes to reach the final goal that the mode specific code is completely separated. Are there any overlaps with your plans? Would like to avoid conflicts during merging. |
That is where I would like to go. I wrote the code originally as a sketch not knowing how much attention it would actually get, and shortcut a few things to get it in time for the competition. Now there is no rush, what would be great is moving each mode into a derived class as you have done and push the html for each mode into the class too. I have had a few thoughts on this, and I think the way I see it happening is to store a F string in each class of the specific HTML then send that via sockets to a statically built web page. I don't want to use a dynamically built page as its harder to maintain, or at least this is what I have found in previous projects. So having a generic page that can be edited and debugged separate of the ESP is the way I have built it currently and the way I would like to keep it. Other than that its all open to ideas. |
I just got involved in a more extensive refactoring. My goal was to centralize the implementation of each mode and encapsulate all mode-specific code in the mode classes, which now already contain the rendering and processing of the config.
I've tried to make the change in manageable commits, so hopefully it's easy to understand.