-
Notifications
You must be signed in to change notification settings - Fork 55
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
New feature that allows to limit the max power from the Grid #220
base: v1.2.1
Are you sure you want to change the base?
Conversation
PIDTWCManager=open("/home/pi/TWCManager/TWCManager.pid","w") | ||
PIDTWCManager.write(str(os.getpid())) | ||
PIDTWCManager.close() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hardcoded path would fail on any installation where the directory specified doesn't exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right, it should be changed for:
PIDfile=config["config"]["settingsPath"] + "/TWCManager.pid"
PIDTWCManager=open(PIDfile,"w")
PIDTWCManager.write(str(os.getpid()))
PIDTWCManager.close()
Should I submit a new PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can push a new commit to your branch, and that will then update the changes in the PR :)
setup.py
Outdated
|
||
setup( | ||
name="TWCManager", | ||
version="1.2.1", | ||
package_dir={"": "lib"}, | ||
packages=find_namespace_packages(where="lib"), | ||
packages=find_packages(where="lib"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would break the setup routine - as we use the namespace convention for our libraries. What led you to change this out of interest? Did you get an error previously?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it didn't work, I get an error and couln't find any way to solve it but to remove the namespace:
"ImportError: cannot import name 'find_namespace_packages'"
You should reject this change, don't know why it didn't work for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably means that you are aither running setup.py with python2 or your setuptools is too old. In that case try pip install --upgrade setuptools
(Maybe use pip3 instead of pip)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already did it but it didn't work, I'll give it another try anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get the same error, I going to give up on this...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't stress, it probably doesn't matter too much in your instance as it's working for you. As @tjikkun has mentioned, namespace packages were introduced in a later version, so if you see an error that says that the function can't be found, that is why. The problem is our modules are not in a traditional (non-namespace) format, so while what you changed it to might run on your older version, it would actually break newer python installs that do support namespace format because it would skip over all of the modules.
If you want to roll back the change to this file in your branch (limit_amps_from_grid) that you've submitted a PR for, you can do this:
git checkout v1.2.1 -- setup.py
git commit
git push
And the change should be reverted from your PR.
Hi @juanjoqg, just want to check in on this PR - how do you think it is placed, do you feel that it is ready for a final review for merge or is it still a work in progress? |
Hi @ngardiner, I've been using it and I'm pretty confident it's working fine, the only potential conflic is on "setMaxAmpsToDivideAmongSlaves", with the last fix I pushed, it should not interfere any other scenario. From my side the feature is done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested, the code seem to work as intended.
But with that said I am personaly interested in a function that always limits the charging to a max grid limit minus consumption, regardless of policy.
I do think there is a valid use case for running twcmanager without solar just to dynamically protect the consumption fuse. twcmanager also provide scheduling, and maybe in the future smart use of dynamic prices for electricity.
I do think the setup of this grid limiting mode with extending policies is a smart way to do it. But as a end user it would have been simpler to just set the maxAmpsAllowedFromGrid and that would apply to all policies.
There is a setting choice for "Non-Scheduled power action", it would be neat to also have a option to select "Charge at grid limit". I guess it would need another policy defined to handle that.
def check_max_power_from_grid(): | ||
global config, hass, master | ||
|
||
# Check solar panel generation using an API exposed by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think this comment about HomeAssistant API is missplaced, (also in the function that it was copied from). Remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, you are wright, copy&paste issue...
|
||
# Calculate what we should max offer to align with max grid energy | ||
amps = self.config["config"]["maxAmpsAllowedFromGrid"] + \ | ||
self.convertWattsToAmps(generationW - consumptionW) + \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a solar production that is connected to a own fuse, Consumption has a smaller fuse. So the solar production is not really helping to increase the maxAmpsAllowedFromGrid. I guess this case needs to/could be added as a setting to ignore generationW.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be controled latelly by the wiringMaxAmpsAllTWCs and wiringMaxAmpsPerTWC, despite of the value given to the maxAmpsToDivideAmongSlaves it's always limited to the wiring limit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But wiringMaxAmpsAllTWCs and wiringMaxAmpsPerTWC are hardcoded, right?
I'd like the charge power to be dynamic based on the other consumers in the house. So if the rest of the house consume 5A out of 16A, the car can still charge with 9A.
It was the less "invasive" way to add this feature. I don't understand your "Non-Scheduled power action", you mean order charging with grid limit when no other action is required? I do not see this use case. Could it be the same as schedule charging for every day from 0 to 23? |
Yes, I understand why you did it like that. |
I would love to have such a "load balancing" feature working in the release! I'm not sure if I fully understand the implementation discussed above, but I think the monitoring whether the max current from the grid is drawn should be done at all times when charging is done, independent of any active policy. Btw: I have a DSMR-meter that measures the incoming and outgoing currents (3 Phases) through the main fuses of the house. I have that available in Home-Assistant so could directly provide this data to TWC. So TWC wouldn't even have to calculate this using the consumption + the actual charge current. So the only thing that I would like TWC to do is to check if the value of 1 of the phases surpasses (in my case) 25Amps and then reduce the charging accordingly. An alternative would be if I could have HomeAssistant to instruct TWC to reduce to wiringMaxAmpsAllTWCs dynamically. Is that how this new feature is going to work? |
I do think that you should use the HASS module to query consumption values from Home Assistant. (My consumption is measured using a meter connected to my fronius inverter. ) I think it would be a simpler configuration to make the grid consumption limit active regardless of active policy. I have been running the solution for some time and I think it should work for your use case if you adapt the configuration as described. Maybe I could take a shot at creating a solution without the policy if this is the way we want to go? @ngardiner |
Yeah, that is what I meant. I already have use the HASS module to query the consumption from Home Assistant, as it is being used to calculate the solar surplus during "Track Solar". |
This PR is impacted by #331 - Just a heads up for now. |
I did a rebase of this PR in #384 , it needs testing. |
I must say this is a poor man's load balancer. It relies on power usage in Watt and calculates Amps using the simple convertWattsToAmps function. Many smart power circuit meters offer actual Amps which should be supported IMHO. My grid usage is not very balanced among the 3 phases so convertWattsToAmps will certainly get it wrong and calculate the headroom too high. |
Fully agreed from my perspective. I do have a meter on my main power to the house that independently measures the Amps for each of the 3 phases. So I would really like to be able to provide the Amps directly. I could live with the ability to only provide the Amps for 1 phase only, in that case I would dynamically provide the Amps for the phase with the maximum current. |
I fixed the "poor man's load balancer" in #384. I suggest we continue the discussion there. |
Enable this new feature through policy configuration in the config.json file.
Also a supervisor feature has been added to allow run the TWCManager with screen and restart it in case of crash