SmartEVSE / SmartEVSE-2

Smart Electric Vehicle Charging Station (EVSE)
86 stars 42 forks source link

Solaredge support #6

Closed maximevince closed 3 years ago

maximevince commented 3 years ago

This pull request adds SolarEdge support to SmartEVSE.

This allows you to use a SolarEdge inverter's Modbus-RTU interface, when configured as "SunSpec (Non SE logger)". (E.g. using SetApp). The SolarEdge inverter has to have a meter attached to it, that allows you to measure mains current consumption. (E.g. the SE-WNC-3Y400-MB-K1 Modbus meter).

Since SunSpec defines many values as int16, and not as int32, I had to add a "len" field to the meter configuration. It's not perfect yet, since SunSpec does define some values as int32, and currently I defined a single "len" field for all of the meter field. This is enough to get useful current measurements from the inverter, though. I did not test the voltage / total power / total energy readings yet. Total energy is known to be bogus (because it will be interpreter as 16bit instead of 32, as described above).

Let me know if you'd like to see some things differently.

bobosch commented 3 years ago

To save variables and function parameters we should use the flag "bool IsDouble" and rename it to "unsigned char DataType" (0: int32, 1: double, 2: int16)

@mstegen I can't assess the XC8 parameter settings - what do you think? And I also can't say which way is better in combineBytes - the old static way or the for loop. The static way is better readable and as every byte is processed by this function I think it is also faster. As the word swap is not necessary on int16, I suggest to use an additional if statement to handle int16

The Finder electric meter uses 4 word (8 bytes!) for some measurements - I was able to avoid this by using the register for double values. A data type definition per register would be more flexible, but configuration would be more complicated - so I tried to avoid that.

maximevince commented 3 years ago

Hi @bobosch, thanks for the fast reply.

I have updated the pull request taking your feedback into account:

maximevince commented 3 years ago

Okay, apparently the scaling factor on SolarEdge can change dynamically. I need to update the code for that, too. Expect another update. But meanwhile, you can review this already.

bobosch commented 3 years ago

Hi, great thanks for your contribution!

Because of backward compatibility in custom electric meter settings I suggest to use 0: int32, 1: float32, 2: int16

And I noticed that I forgot to reserve a modbus register to set the function ... I would switch

#define MENU_EMCUSTOM_READMAX 27                                                // 0x020F: Maximum register read (ToDo)
#define MENU_EMCUSTOM_FUNCTION 36                                               // 0x0218: Modbus Function (3/4) of custom electric meter

as function is more related to endianess/datatype. Please also increase constant MODBUS_SYS_CONFIG_COUNT by 1 to allow 0x0218 to be read/set

The function should also default to "4". When reading from eeprom it could be every value - please add a line in function validate_settings to ensure a correct value.

maximevince commented 3 years ago

Okay, the dynamic scaling is implemented and seem to be working nicely. I still need to update the pull request. I also took your last comments into account.

Before I push the update, I have a question about the minimum charge current, which is set at 6 A. Is there a specific reason for this? My car seems to be able to start at 5A min, but that's currently not an option in the menu's (except when changing the code, of course)

mstegen commented 3 years ago

Thanks for your contribution.

Ref the minimum charge current, the minimum you can set according to the J1772 spec is 6A. (max is 80A) Anything lower is not supported and will result in a charging error. Some EV's allow a lower setting from inside the car (for example a tesla).

maximevince commented 3 years ago

Hi @bobosch, @mstegen, thanks for the feedback. PR has been updated. -> Implemented SolarEdge dynamic scaling factor (for current measurements only) -> Left the minimum charge current at 6A according to J1772 -> Switched MENU_EMCUSTOM_READMAX and MENU_EMCUSTOM_FUNCTION -> Increased MODBUS_SYS_CONFIG_COUNT -> Set MODBUS FUNCTION default value to '4'

bobosch commented 3 years ago

Most looks good for me, except the unfinished Power/Energy request/receive. When there 32 bit values, is there still a scaling factor? If not, It would be enough to hardcode the use of 32 bit values in the switch statements...

Could you please link here the modbus register documentation of the SolarEdge?

maximevince commented 3 years ago

Update is coming soon. Here's the link to the Modbus spec: https://www.solaredge.com/sites/default/files/sunspec-implementation-technical-note.pdf

maximevince commented 3 years ago

Okay, update pushed. Let me know if you'd like some details changed.

AndrejValand commented 3 years ago

This is interesting addition, to use the SolarEdge meter if it is already there... If I understand correctly after reading the SolarEdge documents an Inverter with two RS485 ports is needed? Either native or an older inverter with an upgraded additional RS485 port for this to work?

PA1RB commented 3 years ago

The following is not to critique development work, but I actually wonder, conceptually and architecturally, why the smartEVSE system should be aware of the amount of power generated by a PV-inverter?

What I assume one wants to achieve, is to feed as much as possible any surplus power generated into your car batteries instead of feeding it back into the grid.

That function is actually achieved by measuring the net power consumption at the utilities meter point, as the Sensorbox-2 does.

Measuring power produced by a PV-inverter does not take other power consumers or other PV-inverters into account?

For sure the power generation info of a solar installation is relevant information, but should it not be monitored by a dedicated PV-inverter management system, and/or a higher-order home (or building) automation system?

AndrejValand commented 3 years ago

The following is not to critique development work, but I actually wonder, conceptually and architecturally, why the smartEVSE system should be aware of the amount of power generated by a PV-inverter?

As I was studying the details further the SolarEdge meter measures the consumption at the utility meter point like the Sensorbox-2 does. And for people that have SolarEdge inverters with this additional meter this would be interesting as this eliminates the need for an additional meter like Sensorbox-2 and additional set of current transformers. If I understand all this correctly? If Yes, I think it is a great idea.

PA1RB commented 3 years ago

OK, got it now. Found the explanation in SolarEdge Installation Guide Energy Meter with Modbus Connection

maximevince commented 3 years ago

Apologies for being so slow to respond. Thanks for merging!

bobosch commented 3 years ago

Thank you for contributing!

devdems commented 7 months ago

@maximevince would it be possible to describe how have you connected the SolarEdge inverter to SmartEVSE? Which pins did you need to connect? Tnx!

maximevince commented 7 months ago

@devdems You have to configure the SolarEdge inverter for ModBus logging using RS-485. You can find more information here: https://knowledge-center.solaredge.com/sites/kc/files/sunspec-implementation-technical-note.pdf And here: https://github.com/WillCodeForCats/solaredge-modbus-multi/wiki/Configuration