Master - Githubissues

RobbieDemaegdt commented 1 year ago

I have added the Device_Id so I can use other plants than only my fixed first one. Currently you just give a number, default is 0. I also added a lot of text to remove the random numbers. I also added the griddirection as this wasn't available yet. Also cleanup the code as there was 3 times the code where isAlarm was being a value assigned.

djansen1987 commented 1 year ago

HI @robshot

Thanks! Looks good at first sight. I will test the code with some plants that have only 1 plant, have a SEC option, or have batteries like the H1.

What might become an issue is that some use isAlarm and other don't have this and use todayAlarmNum. Will test if this will work and if this is compatible will merge the pull request

RobbieDemaegdt commented 1 year ago

Ok thanks, first I was not going to do the pull request as I have never done this before on github but when I saw your update on the code I checked how to do the pull request.

The only problem I have I couldn't setup a test HA with docker on windows to development, do you have this and how did you installed it? I followed the instructions on the HA website but they didn't work for me.

djansen1987 commented 1 year ago

HI Rob, Could you explain how the CONF_DEVICE_ID needs to be used ? And is the change working good for you and are there any breaking changes for other users when installing this update ? What will happen for example if the CONF_DEVICE_ID is not configured or when left empty ?

faanskit commented 1 year ago

In a few weeks I'll get my second plant set-up. Had planned to implement something like this, but instead I have now reviewed it.

From my review, I have some comments and possibly some answers to your questions @djansen1987 .

1. Could you explain how the CONF_DEVICE_ID needs to be used ? I think this proposed code makes an incorrect/stretch use of the configuration parameter device_id. It is defined in core/homeassistant/const.py: CONF_DEVICE_ID: Final = "device_id"

device_idis a bit special in Home Assistant, so not optimal to repurpose it like this.

Maybe add an own configration instead: CONF_PLANT_ID: Final = "plant_id"

It looks as it the configuration.yaml shall include the following:

sensor:
  - platform: saj_esolar
    username: aa@bb.cc
    password: abcd1234
    device_id: 1    # Or a number representing the plant in API /getUserPlantList response. 0 is default
    resources:
      - nowPower

In my revised proposal ==> plant_id: 1 # Or a number representing the plant in API /getUserPlantList response. 0 is default

2. And is the change working good for you and are there any breaking changes for other users when installing this update ? Don't know yet. Will try something in a few weeks. From my review:

Configuration for todayAlarmNum nees to change to isAlarm
If there are automations created around todayAlarmNum it will break. Instead reports status in isAlarm
If there are automation created around gridDirection it will break. Will report "Exporting" instead of "1". Unclear what is reported when energy["storeDevicePower"]["gridDirection"]has any other value than "1". Nothing is reported.
If there are automation created around isOnline. Used to report 0/1, now reports "Yes" or "No"
If there are automation created around runningState. Used to report 0/1, now reports "Yes" or "No"

3. What will happen for example if the CONF_DEVICE_ID is not configured or when left empty ? It will default to 0: vol.Optional(CONF_DEVICE_ID, default=0): cv.positive_int, In my revised proposal ==> vol.Optional(CONF_PLANT_ID, default=0): cv.positive_int,

RobbieDemaegdt commented 1 year ago

So the conf_id is just a number, normally it's the same order as what you see on the webinterface, just starting 0 based. @djansen1987 I don't have any issues with my version on my HA, everything works like before just that I have the correct plant.

faanskit commented 1 year ago

Hey,

I just pushed a variant (#36) to this pull request which is a bit more complete and are not doing any breaking changes. Since this pull introduces breaking changes, and make use of device_id in a funky way - I though it was easier to create a new one.

This pull also fails to upload correct information from planList, i.e. currency, plantuid, plantname, isOnline and address. This is addressed in #36.

I also added intalled power as a new resource (systemPower)

/Marcus

faanskit commented 1 year ago

Hey,

I found another thing to be odd. I don't this code will work. plantDetails does not include a parameter called isAlarm.

            if self._type == 'isAlarm':
                if 'isAlarm' in energy['plantDetail']:
                    if energy['plantDetail']["isAlarm"] is not None:
                        self._state = (energy['plantDetail']["isAlarm"])

This is what is available in plantDetail json in my system:

    'plantDetail': {
        'type': 0,
        'runningState': 3,
        'nowPower': 0.0,
        'todayElectricity': 0.0,
        'monthElectricity': 677.81,
        'yearElectricity': 6950.54,
        'totalElectricity': 6952.57,
        'income': 1390.51,
        'todayGridIncome': 0.0,
        'devOnlineNum': 0,
        'devTotalNum': 1,
        'totalPlantTreeNum': 12.33,
        'totalReduceCo2': 6.93,
        'todayAlarmNum': 0,
        'lastUploadTime': '2022-10-26 17:59:20',
        'userType': 2,
        'snList': ['----'],
        'energyCompareYearList': ['2020', '2022']
    },

The current implementation, that makes use of the parameter todayAlarmNum looks to be more correct than what is proposed. Only problem is that is has three instances implemented (copy-paste error I suppose).

If we want a textual representation of alarms, I think it's better to complement than to replace. And the code needs to validate against 'todayAlarmNum'; e.g. (not tested).

            if self._type == 'isAlarm':
                if 'todayAlarmNum' in energy['plantDetail']:
                    if energy['plantDetail']["todayAlarmNum"] is not None:
                            if energy["plantDetail"]["todayAlarmNum"] > 0:
                                self._state = "Yes"
                            elif energy["storeDevicePower"]["gridDirection"] == 0:
                                self._state = "No"
                            else:
                                self._state = "Unkown"
                                _LOGGER.error(f"Alarm number NEGATIVE: {['plantDetail']["todayAlarmNum"]}")

RobbieDemaegdt commented 1 year ago

Yes those 3 alarms are already cleaned up I think in my commit, unless I missed a few. I should indeed be parallel included, as some people like the numbers other like more a textual interface.

djansen1987 commented 1 year ago

HI @robshot,

Thanks for your contribution to the project. I have merged your PR, later also merged the PR of @faanskit which had simliarities but also conflics with your pr. I have merged them to what i think will work best for the users. I have created a pre-release/beta of this.

Could you maybe both test is and let me know if it works for you so i can make it in a stable release ?

RobbieDemaegdt commented 1 year ago

Do I have to do something to see the beta release? I'm still on the 1.2.9 and the only option I have is to update to 1.3.1 but that is without the multiple plants support so I want do upgrade directly to the beta version.

djansen1987 commented 1 year ago

Do I have to do something to see the beta release? I'm still on the 1.2.9 and the only option I have is to update to 1.3.1 but that is without the multiple plants support so I want do upgrade directly to the beta version.

If beta is turned off in HACS you can enable it by going to the integration in hacs, click the 3 dots and then click redownload. From here you can enable beta

faanskit commented 1 year ago

The multi-plant code seems to be operational for my two sites.

This software version introduces breaking changes, and not all of it works. I could only test a system with inverters.

If todayAlarmNum is configured in configurations.yaml, the system will not pass configuration change. invalid

The documentation is not updated to reflect this. README.md still mentions todayAlarmNum, which needs to be changed to isAlarm

Also, the implementation of isAlarm looks to be corrupt. When used, ut reports an unknown value. isalarm

The code appears to be looking for isAlarm in plantDetails, which does not exist

            if self._type == 'isAlarm':
                if 'isAlarm' in energy['plantDetail']:
                    if energy['plantDetail']["isAlarm"] is not None:
                        self._state = (energy['plantDetail']["isAlarm"])

I suggest to revert back to original code, and add the following for 'isAlarm'; ie. keep todayAlarmNum and add isAlarm

             if self._type == 'isAlarm':
                if 'todayAlarmNum' in energy['plantDetail']:
                    if energy['plantDetail']["todayAlarmNum"] is not None:
                            if energy["plantDetail"]["todayAlarmNum"] > 0:
                                self._state = "Yes"
                            elif energy["plantDetail"]["todayAlarmNum"] == 0:
                                self._state = "No"
                            else:
                                self._state = "Unkown"
                                _LOGGER.error(f"Alarm number NEGATIVE: {['plantDetail']["todayAlarmNum"]}")

RobbieDemaegdt commented 1 year ago

I updated to the beta release and everything is working fine

djansen1987 / SAJeSolar

Master #32