Update inverter.py #47
Conversation
To allow for direct LAN API access using X-Forwarded-For header.
|
hey @squishykid any ideas when this might be merged/accepted please? |
|
Yes please, keen as mustard! |
|
I dont think it is inverter specific as such. More so specific to the use of the pocketwifi v2 adaptor. It is a fix to allow direct communication without needing a pi etc in the middle acting as a proxy. |
|
Sorry, what I am trying to say is that some inverters do not require this header, such as the X1 Hybrid which I was able to test the initial version of this library with. My concern is that not all inverters will be able to handle the addition of the header and this might cause a new bug. Therefore I only want to enable this feature for the specific inverters where it has been identified that this will improve functionality. |
|
I have one X1 Boost and one X1 hybrid (both with pocketwifi v2 adaptors) and I needed to do this for both. Its the newer pocketwifi firmware solax released that broke this originally. Then a workaround is using the header, which they have patched out, which is why I posted the firmware that allowed this header method to work also. |
|
I dont think this is inverter specific at all. It relates to the pocketwifi adaptor v2. |
|
maybe we could make the header field an option then? that of which is defaulted to none such that it doesnt break any current deployments users may have? |
|
is this likely to be merged? do I need to further explain that this is NOT inverter specific as it relates to the pocketwifi v2 adaptor? how can we move forward with this? |
|
I am happy to alter the PR in whatever manner is deemed appropriate. I can add the header just for the X1-Boost and X1-Hybrid (the inverters I physically have) if thats what you wish, however it is not an inverter specific fix as it comes down to what inverter the user is using the solax pocketwifi v2 adaptor on. |
|
tested last commit and does work for X1 hybrid and X1 boost only. Awaiting your further advice Mr @squishykid |
|
Hi @dvisser, very happy to have this type of change included. I think the issue at the moment is just implementation specific. I want to stress that it is important that we don't modify existing behaviour unless a user chooses to opt in, or if the library can detect which version to use. Maybe we can take a look at adapting the |
|
@squishykid sure, could we look at the last option? I think a parameter (even a simple xffheader = true) that is defaulted to false would suffice. If you agree I can modify and push an updated commit to this PR? I would really like this parameter available via your home assistant solax platform also. |
|
something like my last commit @squishykid ? |
|
Yeah! That's on the right track. We will need to update the tests so they exercise the new code path. I might have some more time next week to look at this properly if you get stuck with that. One thing to note in your change is that you require the user to opt in and out of the feature by changing the parameter. There's a bunch of overdue work in home-assistant for the solax integration which means it's difficult to add new parameters. So ideally we would have the |
|
@squishykid OK awesome. Well if you dont get time next week, please let me know and I will happily do some more dev to get this sorted. I assume we would just try to discover via the current manner and then if that fails, then try again with the xffheader turned on? or something along those lines? I am keen if you want help with the home assistant solax integration also, so please feel free to give me a run through and I can help there too if you wish? |
|
@squishykid something like that I am thinking? after the first attempt at discovery fails, the discovery method then tries again with the X-Forwarded-For header. |
|
may need to make xffheader global, as when discovery sets it to true, it should remain true for all other methods if it was successful. |
|
As mentioned above, I want to introduce this new feature via the 'discovery' mechanism. You can see in #52 for how this was implemented. So we will need some API call examples for the test cases, as described here: https://github.com/squishykid/solax/wiki/DiscoveryError |
|
OK @squishykid I will take a look at #52 and see how I go. |
|
I will try to revisit this asap, as I really want to remove my automation that patches this code in home assistant everytime there is HA core update. Have you had any time to look at this @squishykid ? I am not familiar with the discovery stuff, but I will try to learn it quickly. |
To allow for direct LAN API access using X-Forwarded-For header.