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 17 forks source link

Add an option to preserve properties of original message #22

Closed avk999 closed 8 months ago

avk999 commented 8 months ago

for example I have a few TRVs and have an instance of PID per each to control them. I sent messages to respective PID node but then I'd prefer to have a single scaling/reformatting function and call_service node. For this I need to preserve original properties.

colinl commented 8 months ago

Thanks for this PR

I don't think there is a need to add an option for it, in fact it could be argued that this is a bug fix as nodes should ideally retain other properties anyway. Though I will likely release it as a major version as it could possibly be considered a breaking change. You shouldn't change the version in the PR though, that will be done at release.

There seem to be a large number of whitespace changes, that makes it difficult to see what the changes are. Could you re-do the PR without the whitespace changes? Also anything that changes and changes back again (such as the rename to PIDMSG) should not appear in the commits.

Does that mean that in fact all it needs is the message merge?

colinl commented 8 months ago

In fact I will have to release it as a breaking change because the spread operator, which is ES6, needs at least nodejs 14, I believe.

colinl commented 8 months ago

Also please start a contributors section in package.json and add yourself in, unless you would rather not.

avk999 commented 8 months ago

Hey, Actually I thought you dropped the project long ago. Thank you for the node, it works surprisingly well for such a simple PID implementation. I think the option should still be configurable with the default to off - just to avoid the breaking change.

I'll make a cleaner PR in a few days, avoiding the spread operator. I'm not a JS developer, so don't know those details.

avk999 commented 8 months ago

Closing for now

colinl commented 8 months ago

Don't make it configurable, please.

It should be possible to add the new commit to this PR, rather than start a new one.

colinl commented 8 months ago

Moved to PR #24