MorneSaunders360 / Solar-Sunsynk

Monitor your energy generation, storage, and usage data using an unofficial API from Sunsynk
MIT License
9 stars 2 forks source link

Failed to authenticate with Solar Sunsynk. Please check your username and password. #3

Closed Nurgus closed 1 year ago

Nurgus commented 1 year ago

Triple checked I'm using the right credentials. Is it possible you're only connecting to Region 1?

MorneSaunders360 commented 1 year ago

Ahh, I see there is two regions. I tried to use region 2 with my credentials and it won't log me in. When selecting region 1 my credentials work. I see that they are using two different API endpoint for each region.

Region 1 Uses pv.inteless

Region 2 Uses api.sunsynk.net

Looks like they are starting to transfer all of their old client from region 1 to region 2. Will have to wait for my credential to be available on region 2 to update the integration for home assistant.

If you are willing to assist me, I can ask you to get me the new responses for region 2? And then I can prompt the user as ask which region they are and use the correct endpoint until all users are on region 2 and we don't need the to use region 1 anymore..

IMG_0992

Nurgus commented 1 year ago

Yeah I'd love to help, tell me what to do.

MorneSaunders360 commented 1 year ago

@Nurgus I've successfully incorporated the two regions into our integration. Given that the endpoint for Region 2 is presumably the same, I've proceeded with an update. Would you kindly verify whether the authentication process is functioning smoothly? Furthermore, if everything is working correctly, could you please confirm whether your sensors reflects the respective updates

Nurgus commented 1 year ago

Yeah! That's done the trick, I can see 44 sensors!

First glance feedback: the default entity names are too short. Names like "sensor.soc" and "sensor.address" are very ambiguous and prone to conflicts. Can I suggest "sensor.sunsynk_\<plantid>_soc" or similar?

MorneSaunders360 commented 1 year ago

I'm thrilled to hear that you can now see all 44 sensors!

I appreciate your feedback about the default entity names, and I absolutely agree with you. Having unique and more descriptive sensor names will significantly reduce ambiguity and the chance of conflicts.

I'll proceed to update the naming convention as per your suggestion. I believe "sensor.sunsynk_soc" and "sensor.sunsynk_address" will give us more clarity and reduce confusion. In this format, the plant ID will help identify the specific source of the data, which will be particularly helpful in environments with multiple devices.

Please allow me some time to implement this change, and I will notify you as soon as it's done.

Thank you once again for your valuable feedback, and please feel free to reach out if you have any more suggestions or concerns.

Nurgus commented 1 year ago

I'm over the moon to have this working, thanks so much for doing it.

Bit more feedback. If you give the numeric sensors "state_class: measurement" then they'll show up properly as numbers in graphs and more-info rather than as a series of discrete events.

For power related sensors if you set device_class: battery or device_class: power

then they'll get the appropriate icon, changing battery status and all that good stuff set automatically by home assistant.

Finally unit_of_measurement: '%' or kW or W or whatever.

You probably know all this stuff, I'm literally just learning it now as I'm making some template sensors.

MorneSaunders360 commented 1 year ago

I'm thrilled to hear that the integration is working well for you, and your feedback is most welcome!

  1. Adding the "state_class: measurement" attribute to numeric sensors is a great idea. This will improve the display in graphs, and I'll aim to implement this in the next release.

  2. Setting the "device_class" to "battery" or "power" where relevant is an excellent suggestion. The automated icon adjustments and dynamic battery status tracking will certainly enhance the user interface.

  3. Including a "unit_of_measurement" attribute for each sensor will provide more clarity and context for users reading the measurements. I'll ensure this is incorporated.

I'll make the necessary changes and push them to the repository soon.

Feel free to continue sharing any more feedback or suggestions you may have.

Nurgus commented 1 year ago

Looking great in the latest version. I've got more feedback, I'm starting a new issue.