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

Duplicated registers in Inverter and StorEdge #110

Open
milkotodorov opened this issue Jan 15, 2025 · 10 comments
Open

Duplicated registers in Inverter and StorEdge #110

milkotodorov opened this issue Jan 15, 2025 · 10 comments

Comments

@milkotodorov
Copy link

milkotodorov commented Jan 15, 2025

The following registers in Inverter class and the ones in StorEdge class are duplicating each other (almost all of them).

Are there any good reasons for that?

@herbi3
Copy link
Contributor

herbi3 commented Jan 15, 2025

It is a fix to prior to the little endian map

Now there's the little endian map for the items that require it under inverter.

@milkotodorov
Copy link
Author

milkotodorov commented Jan 15, 2025

We have in the Inverter class it is a BIG endian and in the StorEdge class it is LITTLE endian. I do not get it.
The registers in the Inverter class (with the BIG endian) require the map of the StorEdge class?

When you change the value of these registers then, which one should be used for the change? The one from the Inverter or from the StorEdge class?

@herbi3
Copy link
Contributor

herbi3 commented Jan 15, 2025

The StorEdge class isn't needed now, as the registers that require little endian under the Inverter class now have a lookup map which automatically uses little endian for the registers that need it under Inverter class

@milkotodorov
Copy link
Author

I see...the StorEdge was like a workaround until there was a way that these registers be part of the Inverter class and now no longer needed.

We have however the 0xe000: export_control_mode, 0xe001: export_control_limit_mode and 0xe002: export_control_site_limit not part of the Inverter class.

Why not make them part of the Inverter class and remove the StorEdge one?

@herbi3
Copy link
Contributor

herbi3 commented Jan 16, 2025

It looks like they're still there?imageimageimage

@milkotodorov
Copy link
Author

These ones: https://github.com/nmakel/solaredge_modbus/blob/master/src/solaredge_modbus/__init__.py#L703-L705 - they are not part of the Inverter class.

@milkotodorov
Copy link
Author

Actually they are there, but with different offsets: https://github.com/nmakel/solaredge_modbus/blob/master/src/solaredge_modbus/__init__.py#L555-L557

@milkotodorov
Copy link
Author

Then shall we remove the StorEdge class to not cause confusion, when not needed?

@herbi3
Copy link
Contributor

herbi3 commented Jan 16, 2025

It has zero effect on functionality. You can use either one. It's easier to use the inverter one to call everything at once. This is someone else pull request and ultimately it can be removed should the author be ok with that

@milkotodorov
Copy link
Author

Yes, you are right. But maybe it is still better to cleanup the code from unnecessary class, isn't it?

I'll create a PR, that should be very quick.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants