serkri / SmartEVSE-3

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

3 Phases setting #12

Closed deqw closed 2 years ago

deqw commented 2 years ago

Got version 1.3.1 installed.

$('#enable_3phases').text(data.settings.enable_3phases ? "Yes" : "No");

The setting is: '3phases_enabled' so this will always show 'No'

koen-serneels commented 2 years ago

Should be fixed. See https://github.com/serkri/SmartEVSE-3/releases/tag/v1.3.2-serkri-v3

deqw commented 2 years ago

Updated firmware and spiffs binary to v1.3.2 but now all the settings data is empty on the index page.

koen-serneels commented 2 years ago

Sorry. Should be fixed see https://github.com/serkri/SmartEVSE-3/releases/tag/v1.3.3-serkri-v3

deqw commented 2 years ago

OK, got it and no problem! Here to test. Working fine now with v1.3.3.

deqw commented 2 years ago

I was looking at this again, just a thought. I'm not sure a binary option is the best way to code this setting. Now you have the setting 'enable_3phases', when the value is true you are using 3 phases. But what when it is false? When it is false you are using 1 phase. But that does not show. Using a struct which defines the options could make this clear. It even gives you the possibility to add a third option, eg.: none. I see a lot of const char arrays in the code and for these my thoughts are the same. I have no intention to code this myself (nor to start a semantic discussion about best practices) so I leave this to you. I'm happy with the binaries and it works.

koen-serneels commented 2 years ago

There are only two states: charging 3p (flag = true) or not (flag = false) which is also what this flag in extend controls: the 2nd contactor. False implicitly means charging 1p as there is no such thing as 0p or >3p. The yaml doc also outlines that it's switching between 1p and 3p. Also, this property doesn't say anything about the state, so "none" would not makes sense here. The car could even be disconnected when this flag will still return true. There is already a property to indicate the current state ("state") , so it's better to avoid duplicating this partially into this property. Another approach could have been calling the property "phases" and then an enum "1P" or "3P", but I think don't that is really worth the energy to change this in the contract. Imho it's more of a value add to clarify the yaml contract what true and false means (in case it's still doubtful) and for the other properties introduce an enumeration so it's clear which values to expect

deqw commented 2 years ago

Although 3 phase is correct showing the state, C2 does not switch so only loading 1 phase. Can you take a look?

deqw commented 2 years ago

Found MODE_NORMAL is necessary for 3 phase loading, so C2 switching now ok.

What is the reason for excluding the other modes? Can changing the SmartEVSE setting to 3 phase loading, while the car is connected or already loading 1 phase, cause any problems?