Skip to content
This repository has been archived by the owner on Nov 18, 2020. It is now read-only.

Mode separation #25

Merged
merged 9 commits into from
Dec 2, 2019
Merged

Conversation

LarsMichelsen
Copy link
Collaborator

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.

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.
@thebigpotatoe
Copy link
Owner

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.

@thebigpotatoe thebigpotatoe added the enhancement New feature or request label Dec 2, 2019
virtual void applyConfig(JsonVariant& settings);
};

std::map<String, ModeBase*> modes;
Copy link
Owner

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?

@thebigpotatoe
Copy link
Owner

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).

@thebigpotatoe thebigpotatoe merged commit cfa1ce8 into thebigpotatoe:master Dec 2, 2019
@thebigpotatoe thebigpotatoe mentioned this pull request Dec 2, 2019
@tristndev
Copy link
Contributor

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. :)

@thebigpotatoe
Copy link
Owner

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.

@LarsMichelsen LarsMichelsen deleted the mode-separation branch December 4, 2019 08:06
@LarsMichelsen
Copy link
Collaborator Author

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.

@thebigpotatoe
Copy link
Owner

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants