Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unpack requires a buffer of 4 bytes problem with Deye & Ethernet logger #262

Open
terzo33 opened this issue Nov 14, 2024 · 62 comments
Open
Assignees
Labels
enhancement New feature or request

Comments

@terzo33
Copy link

terzo33 commented Nov 14, 2024

Hi @davidrapan , when i change grid peak shaving value, give me an error

Screenshot_20241114_095100_Home Assistant.jpg

Screenshot_20241114_200601_Home Assistant.jpg

when i used wifi logger it worked fine, now I'm using logger LAN adapter

EDIT: any numeric value I change on your integration gives me the same error

Screenshot_20241114_201933_Home Assistant.jpg

@davidrapan
Copy link
Owner

Hi, can you try the change with debug logging enabled? Thanks.

@terzo33
Copy link
Author

terzo33 commented Nov 14, 2024

Hi, can you try the change with debug logging enabled? Thanks.

yes, I'm starting to record now

even these peaks that go to 0 and then return to the set value is a bug. it didn't happen before

@terzo33
Copy link
Author

terzo33 commented Nov 14, 2024

Hi, can you try the change with debug logging enabled? Thanks.

@davidrapan see log... thanks

home-assistant.txt

those spikes that go to 0 are due to the integration stopping working for 5 seconds and then working again

@terzo33
Copy link
Author

terzo33 commented Nov 15, 2024

@davidrapan 12h log

home-assistant_2.txt

Sometimes integration stop working for few seconds

Screenshot_20241115_083020_Home Assistant.jpg

@terzo33 terzo33 changed the title Grid peak shaving value problem Grid peak shaving value problem with deye Lan logger Nov 15, 2024
@davidrapan
Copy link
Owner

Hi @githubDante, could you take a look at it?

@davidrapan
Copy link
Owner

@terzo33 can you please try to set the value using built-in ACTIONS in Developer tools. Use Modbus function code 0x06.

@githubDante
Copy link

Hi @davidrapan ,

The response after write is empty according to the log, hence the error. It comes from here

You can ignore the response or if the value needs to be verified issue a new read_holding/read_multiple_holding request and return the response from it.

@githubDante
Copy link

@terzo33 can you confirm that the inverter is working according to the selected value despite the error which you see at the bottom of the screen

@terzo33
Copy link
Author

terzo33 commented Nov 15, 2024

@terzo33 can you confirm that the inverter is working according to the selected value despite the error which you see at the bottom of the screen

Yes confirm... works but I see error a the bottom of the screen. Need to fix it

@davidrapan @githubDante

@davidrapan
Copy link
Owner

So it does change the value in your inverter? Then ignore the error for now and I'll try to do something about it later. 😉

Thanks @githubDante!

@terzo33
Copy link
Author

terzo33 commented Nov 15, 2024

@davidrapan when I set the value, I get the error, however after 10/20 seconds the value is set correctly on the inverter. Before with the wifi logger, everything was immediate. Also every now, the solarman integration stops working for a few seconds using the LAN logger.

@davidrapan
Copy link
Owner

when I set the value, I get the error, however after 10/20 seconds the value is set correctly on the inverter.

The writing into inverter works correctly. The error you see is from parsing of the response from inverter. Nothing serious.

Also every now, the solarman integration stops working for a few seconds using the LAN logger.

This is worse... Maybe slight adjustments to the timings of the requesting flow could improve it so you don't see disconnects.

@terzo33
Copy link
Author

terzo33 commented Nov 15, 2024

@davidrapan I hope you can fix it

@davidrapan
Copy link
Owner

In const.py you can try to change rows:

TIMINGS_UPDATE_TIMEOUT = TIMINGS_INTERVAL * 6
TIMINGS_SOCKET_TIMEOUT = TIMINGS_INTERVAL * 4 - 1

To:

TIMINGS_UPDATE_TIMEOUT = TIMINGS_INTERVAL * 8
TIMINGS_SOCKET_TIMEOUT = TIMINGS_INTERVAL * 6 - 1

@terzo33
Copy link
Author

terzo33 commented Nov 15, 2024

@davidrapan done

@davidrapan
Copy link
Owner

Don't forget to restart HA to apply changes and let me know if it had any effect on the disconnects.

@terzo33
Copy link
Author

terzo33 commented Nov 16, 2024

Don't forget to restart HA to apply changes and let me know if it had any effect on the disconnects.

@davidrapan done but it's the same

Screenshot_20241116_112751_Home Assistant.jpg

@davidrapan
Copy link
Owner

Well in that case I don't think we can do anything about it in integration... but it's really strange that it goes to 0W...

@terzo33
Copy link
Author

terzo33 commented Nov 18, 2024

Well in that case I don't think we can do anything about it in integration... but it's really strange that it goes to 0W...

@davidrapan I checked better and there is no connection problem but simply the parameters all go to 0, then after a few minutes they return to the correct values. there will surely be a bug

Screenshot_20241118_102348_Samsung Internet.jpg

@davidrapan
Copy link
Owner

And all the sensors goes to 0 too?

@terzo33
Copy link
Author

terzo33 commented Nov 18, 2024

And all the sensors goes to 0 too?

@davidrapan all values ​​editable

@davidrapan
Copy link
Owner

So the sensors shows what? Value or Unavailable?

@terzo33
Copy link
Author

terzo33 commented Nov 19, 2024

So the sensors shows what? Value or Unavailable?

@davidrapan show 0 values

Screenshot_20241118_102348_Samsung Internet.jpg

@davidrapan
Copy link
Owner

Those are not sensors.

@terzo33
Copy link
Author

terzo33 commented Nov 19, 2024

Those are not sensors.

Sorry, all sensors works fine, show alwasys correct value

@davidrapan
Copy link
Owner

I have honestly no idea why is it happening. It would make sense if also the sensor values did become Unavailable during periods with zeroed configurables...

You switched from Wifi Logger to Ethernet Logger?

@terzo33
Copy link
Author

terzo33 commented Dec 5, 2024

Hi @davidrapan , after upgrade solarman integration I see always this error

Screenshot_20241205_090907_GitHub.jpg

@davidrapan
Copy link
Owner

What does the log say?

@terzo33
Copy link
Author

terzo33 commented Dec 5, 2024

What does the log say?
home-assistant_3.txt

I see some error for "Buffer of 4 bytes error" and then I see on dashboard all editabile value go to 0 already :(

after a few minutes they return to the correct value automatically

Screenshot_20241205_094547_Home Assistant.jpg

@davidrapan
Copy link
Owner

davidrapan commented Dec 5, 2024

Okay for starters you can try change row:

return await self.write_multiple_holding_registers(start, ensure_list(arg))

to:

                _value = ensure_list(arg)
                _LOGGER.warning(f"DEBUG: {type(arg)} - {arg}, {type(_value)} - {_value}")
                return await self.write_multiple_holding_registers(start, _value)

Then restart Home Assistant and look what's in the log after you try change any of the number configurable.

@githubDante
Copy link

I think this is not needed at all. The request is correct, but the response from the datalogger after write is truncated.

Here an example:

2024-12-05 09:47:40.406 DEBUG (MainThread) [custom_components.solarman.api] [2110011481] SENT: 3e 21 00 00 00 09 01 10 01 25 00 01 02 0a f0
2024-12-05 09:47:40.670 DEBUG (MainThread) [custom_components.solarman.api] [2110011481] RECD: 3e 21 00 00 00 04 01 10 01 25
>>> pdu = bytes.fromhex('10 01 25 00 01 02 0a f0')
>>> func = create_function_from_request_pdu(pdu)
>>> func
<umodbus.functions.WriteMultipleRegisters object at 0x7349e5708e80>
>>> func.starting_address
293
>>> func.values
[2800]
>>> func.create_response_pdu().hex(' ')
'10 01 25 00 01'

As you can see two bytes are missing from the end.

@terzo33
Copy link
Author

terzo33 commented Dec 5, 2024

yes indeed solarman gives the error but then it sets the value correctly

@davidrapan
Copy link
Owner

Ah I did not notice that! I immediately assumed the error was in my code. 😆 Thanks!

@terzo33
Copy link
Author

terzo33 commented Dec 5, 2024

Ah I did not notice that! I immediately assumed the error was in my code. 😆 Thanks!

Awesome. Could you fix for next release?

@githubDante
Copy link

This was the case 3 weeks ago ... #262 (comment) 😃

@davidrapan
Copy link
Owner

davidrapan commented Dec 5, 2024

I totally forgot about it and thought it's new. Guess I'm overworked. 😆

@davidrapan
Copy link
Owner

So anyway, I was right and it's specific to Ethernet Loggers.

@terzo33
Copy link
Author

terzo33 commented Dec 5, 2024

@githubDante if you use lan logger. Could you check if these values(all editable values)sometime go to 0?

386339043-7483294c-eca0-4674-8e43-7bde47ed29c6.jpg

Screenshot_20241205_094547_Home Assistant.jpg

@davidrapan
Copy link
Owner

davidrapan commented Dec 5, 2024

Do we have debug log with this happening?

@githubDante
Copy link

So anyway, I was right and it's specific to Ethernet Loggers.

I wouldn't count on that. Can be a firmware bug 😃

@terzo33 sorry but no, I don't use HA at all.

@davidrapan
Copy link
Owner

I wouldn't count on that. Can be a firmware bug 😃

Sure, what I mean is we are not talking solarman protocol (it's in a different league all together and adds bytes instead, haha) here.

@davidrapan
Copy link
Owner

So this is how the reply should look like right?

8e 39 00 00 00 06 01 10 01 25 00 01

@githubDante
Copy link

Yes, that's the correct response. Here a small patch which should fix this

diff --git a/custom_components/solarman/api.py b/custom_components/solarman/api.py
index d59adaf..1ad82f5 100644
--- a/custom_components/solarman/api.py
+++ b/custom_components/solarman/api.py
@@ -101,7 +101,11 @@ class PySolarmanV5AsyncWrapper(PySolarmanV5Async):
     async def write_multiple_holding_registers(self, register_addr, values):
         if not self._passthrough:
             return await super().write_multiple_holding_registers(register_addr, values)
-        return await self._tcp_parse_response_adu(write_multiple_registers(self.mb_slave_id, register_addr, values))
+        try:
+            r = await self._tcp_parse_response_adu(write_multiple_registers(self.mb_slave_id, register_addr, values))
+        except struct.error:
+            r = len(values)
+        return r
 
 class Inverter(PySolarmanV5AsyncWrapper):
     def __init__(self, address, serial, port, mb_slave_id):

Don't know why I can't attach it...

@davidrapan
Copy link
Owner

I made fix: Workaround for modbus tcp responses w/o quantity, try it and if it fails I'll got w/ @githubDante's suggestion. 😉

Anyway, really thanks, Modbus would be sometimes complete madness for me without your help. 😆

@davidrapan
Copy link
Owner

Hi @terzo33, please try latest version and let me know if that bug of printing errors is gone. 😉

@terzo33
Copy link
Author

terzo33 commented Dec 6, 2024

Hi @terzo33, please try latest version and let me know if that bug of printing errors is gone. 😉

@davidrapan yes works 😍😍 but in grid power i see something wrong 😂😂

Screenshot_20241206_211939_Home Assistant.jpg

@davidrapan
Copy link
Owner

davidrapan commented Dec 6, 2024

Please, can you tell me your exact device and model?

@terzo33
Copy link
Author

terzo33 commented Dec 6, 2024

Please, can you tell me your exact device and model?

Deye Hybrid 6kW

@davidrapan
Copy link
Owner

davidrapan commented Dec 6, 2024

I need the model number exactly.

Edit1: It's something like SUN-12K-SG04LP3 (this is mine)

Edit2: And can you show me the same screshoots like in #281 (comment) togerther with screeshots of whole sensor list?

@terzo33
Copy link
Author

terzo33 commented Dec 6, 2024

I need the model number exactly.

Edit1: It's something like SUN-12K-SG04LP3 (this is mine)

Edit2: And can you show me the same screshoots like in #281 (comment) togerther with screeshots of whole sensor list?

SUN-6K-SG03LP1-EU

Screenshot_20241206_213437_Home Assistant.jpg

@davidrapan
Copy link
Owner

And screenshot of Grid and Load sensors? (Voltage, current, power)

@terzo33
Copy link
Author

terzo33 commented Dec 6, 2024

And screenshot of Grid and Load sensors? (Voltage, current, power)

Screenshot_20241206_213835_Home Assistant.jpg

@davidrapan
Copy link
Owner

Mhmmm, gonna have to find out how to detect which device needs scaling * 10 for Grid and Load power and which do not...

Thanks!

@terzo33
Copy link
Author

terzo33 commented Dec 6, 2024

Mhmmm, gonna have to find out how to detect which device needs scaling * 10 for Grid and Load power and which do not...

Thanks!

I'm glad to help. If can help you. before the update the value was correct

@davidrapan
Copy link
Owner

I know, it was requested change because some devices do need it multiplied by 10.

@davidrapan davidrapan changed the title Grid peak shaving value problem with deye Lan logger Unpack requires a buffer of 4 bytes problem with Deye & Ethernet logger Dec 8, 2024
@davidrapan davidrapan added the enhancement New feature or request label Dec 9, 2024
@davidrapan davidrapan self-assigned this Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants