serkri / SmartEVSE-3

Smart Electric Vehicle Charging Station (EVSE)
MIT License
72 stars 26 forks source link

Simplifies configuration #86

Closed ChristianSteffens closed 1 year ago

ChristianSteffens commented 1 year ago

I just started working with SmartEVSE and noticed some issues regarding its configuration.

If needed / expected I can split the different changes into separate MRs.

Furthermore I started renaming and adding some configuration #define. I'm not sure your willing to change them, but I find the current implementation quite error prone. IMHO it would be better to explicitly specify a default, minimum and maximum value for each configuration property and using the allowed values / defines explicitly.

I tried this approach for the MODE, CONFIG, MAINS_METER_MEASURE. Please take a look - I'm happy to apply the changes to the remaining configuration properties as well.

Besides this imminent changes I'd like to propose some other changes and also provide a more detailed README. I did struggle to enable my EV Meter only to noticed (from looking at the code) that I need to explicitly enable the Smart or Solar mode. But that's outside the scope from this MR.

dingo35 commented 1 year ago

Hi Christian,

Thank you for putting effort in improving the SmartEVSE code! Comment on your points:

Also in general we prefer atomic pull requests so we can "pick and choose".....

I´m sorry to be so negative on your PR, I hope I explained it clear enough!

ChristianSteffens commented 1 year ago

the kWh meter's addresses are reserved for Node SmartEVSEs, so this is a really bad idea....

OK, this I understand, but most (if not all) of the meters I use have by default the address 01. But I'll change my meters address.

the possibility to disable the Main Meter: when you disable it in Smart Mode or Solar Mode, you are effectively running in Normal Mode.

Again, this also makes sense. But how do I use only the EV meter in Normal mode? The menu structure only provides the EV meter option in case the mode is Solar or Smart. I don't need any logic in regard to loading, but simply like to track the energy / current during loading.

Using enum instead of defines is even better.

ChristianSteffens commented 1 year ago

The custom component URL is invalid? Or not public?

dingo35 commented 1 year ago

Copy / paste error with the link, I updated it.

As to using only the EV meter and running in Normal mode, one could argue: why wouldn't you just run in Smart mode?

But it really is an architectural question. SmartEVSE started with the assumption that you either have no meters and run in Normal mode; if you add a mainsmeter you can guard your mains breakers by running in Smart mode; if you want to have your energy usage monitored you can add an EVmeter. O wait now we can use that to guard that too, even when multiple SmartEVSEs are used (with load balancing = on). Then we added the web-interface, so now people want to read the values from their meters, even when running in Normal mode.

Very legitimate question, but all the logic written before assumes that Normal mode means no meters or only a Sensorbox connected.... Personally I think now that it is best to be able to disable mainsmeter, and then eliminate Normal mode (why would you not use data on charging when it is available?), but that might be a large operation and also error-prone; I do not have the time currently to do that, and I also would like to know the opinions of my fellow developers, and also from the manufacturer.....