search

colinl / node-red-contrib-pid

A node-red PID loop controller node intended for the control of real world processes
Apache License 2.0
26 stars 18 forks source link

Ability to change PID properties using multiple msg.* values and ability to change bias value #1

Closed nerobot closed 7 years ago

nerobot commented 7 years ago

By allowing multiple msg.* properties (such as msg.setpoint and msg.bias), it's possible to change multiple values in parallel. For example, it allows me to change the setpoint and calculate the PID output in on go.

By allowing the user to change the bias, it makes the node more flexible for different users.

colinl commented 7 years ago

I don't understand what the purpose of the bias is. What does it achieve that initial integral does not? What exact use case does it satisfy? Also, I think that if it is not set to 0.5 then the code stopping the integral from pushing the setpoint outside the proportional region would have to be adjusted and also the code locking the integral. It is better to only do one thing per request anyway, so I suggest re-submitting just the other bits as a separate request. If you modify the code on your branch you can automatically re-submit, you don't need to start again. Then the bias stuff can be a separate commit if we decide it is worthwhile. A minor detail I know but rather than: "The properties below can also be set using msg.---. For example, to change setpoint you can modify msg.setpoint in the message." I would prefer something a little more specific so maybe "The properties below can also be set using msg.---. For example, to change setpoint you can set the message attribute msg.setpoint to the required value." Also there could you clarify whether multiple properties can be set in one message and whether messages of that type will also have a pv value. Finally I think you have forgotten to amend the help text at the bottom of pid.html, unless I have missed it. It is rather tedious having to put basically the same information in two places in different formats, but that is what we have to do I think.

nerobot commented 7 years ago

I agree about not needing the bias. However, it was in the original code, so thought it best to make it more flexible rather than remove it completely. I would prefer to remove it completely.

Removed all reference to new bias from this pull request (thanks for patients, still figuring out how to best use Github).

Documents in folk changed and saved.

colinl commented 7 years ago

Sorry, I think you still have not clarified the fact that a message with msg.--- should also have a pv value, I think that is the case is it not. On the bias, perhaps an example would help to explain why it is pointless. Imagine running the process for a while with the bias set as it is at the moment. Wait for the the process to stabilise so the pv is exactly on the setpoint (assume a perfect process with no noise etc). Now change the bias to zero. What will happen? The power will drop and so the pv will drop by some amount over a short period. Leave the process alone, what will happen? The integral will compensate and bring the process back onto the setpoint. So, what is the point of having a variable bias? You suggested removing it completely, but even that would have no effect, by the time you worked through all the other stuff that would have to change, the integral lock and the calculations to the Initial Integral to work as documented you would find that you were just adding the 0.5 in again in the calculation for the Initial Integral. Read the definition of Initial Integral and you will see what I mean.

nerobot commented 7 years ago

Sorry, I didn't realise you were requesting an update in the readme about needing a PV value in the message. That is important so I have added that.

colinl commented 7 years ago

Merged, thanks.

By the way, I think it is better to make changes to your fork on a branch rather than on master, I think you may find it problematical now trying to merge my master back into yours, if you need to do that.

colinl commented 7 years ago

@nerobot could I ask you to download the latest source from here please and make sure it works ok with your mode of configuring. Then I will release a new version on npm. Thanks.

nerobot commented 7 years ago

Downloaded latest sources from here into my system and it appears to be working fine. I will leave it running for a while and let you know if I notice any issues.