node-red-nodes
node-red-nodes copied to clipboard
node-red
Feature to set frequency for pwm output in gpiod node
Feature is optional, so old config should still work. Depending on sample rate setting in gpiod real values can vary.
Hi, has this been discussed on the mailing list or slack? As per our contribution guidelines that is our preferred starting point for any feature request or proposal.
Coverage remained the same at 66.156% when pulling 26b9588ba6190985bebccff4673d005674173538 on Psykoman1990:feature-setfrequency-for-pwm into 84a445d9d868ff6ecbfdc008fd650127df7ec35c on node-red:master.
Coverage remained the same at 66.341% when pulling d177d6d7a50f882bc928bdfcf46f371e5ab1fe48 on Psykoman1990:feature-setfrequency-for-pwm into 8d45e85acfa10b53fb94b1d6bfc16b9f8cc39eea on node-red:master.
Have you checked the underlying library ? https://github.com/miketrebilcock/js-pigpio/blob/master/js-pigpio/index.js Is only seems to allow certain frequencies to be used. If so then it should be a select box rather than an input field. Also no need for the check box as long as the default is what it is today. (Hence why a discussion would have been good beforehand...)
@dceejay That's why i wrote the note. It's ok to set another value but gpiod will round it to the next valid value. As we don't know what the user is passing as option when starting the daemon (-s option), we don't know which values are appropriate for a dropdown.
@knolleary Hi, no yet. As we just started discussing here, should we still went over to the mailing list?
As you started here, stay here. Just pointing out the contribution guidelines.
In fact, I realise we haven't set the pr template in this repo which would have made it clearer... Will fix that up.
@dceejay I just checked if there's any easy possibility to get the sample rate but didn't found one. So i could make a dropdown with all values possible on all sample rates or leave it as it is. I added the checkbox because of the same reason, as the default value of the pwm frequency depends on the selected sample rate.
Sorry that i somehow missed that in the contribution guidelines.
OK - as long as it handles "any" number and picks the closest automatically then that would be better (rather than trying to get into lists based on sample rate etc... - just setting a frequency would be much easier) If that is the case then again the default frequency should be OK to set as it should then then work out the correct divisor based on the set sample rate ? (so no checkbox needed ?)
It seems that the default rate is the 6th rate (1-4000, 2-2000, 4-1000, 5-800 , 8-500 , 10-400). As for the default rate 5 leads to a default frequency of 800Hz, that would be the value to choose. This will lead to to following real values: (1-800, 2-800, 4-625, 5-800 , 8-625 , 10-800). If that is acceptable then i will remove the checkbox? One thing that i could maybe improve, is to read out the real frequency and show it in the status of the node (e.g. 50@800Hz)?
not sure what you mean by 1-800, 2-800 etc... what are the frequencies ? I prefer we keep the status for the actual reading of the node so in PWM mode that should be between 0 and 100% duty cycle. We could show the actual frequency configuration back in the edit config info tips area when in PWM mode if you like ?
Format: Selected sample rate - Frequency in Hz Is it ok to set a value of Config UI (pi-gpiod.html) from the node logic (pi-gpiod.js)?
as usual .. it depends :-)... it can set defaults, provide suggestions etc... but if a user sets something it shouldn't then get changed without the user explicitly changing it. (Ie it shouldn't change just by re-opening the config page)
I asked because the frequency is only changed when the node is deployed or started. But i think i will split it of the node handling if it's possible (need to take all changes like ip, port into account).
So only think is how the format of this info should be. Do you prefer text like info text or is it ok to add an input field which is not writeable (maybe next to frequency field)? I will check that and tell you if i will do it.
Should i remove the checkbox or not in despite of the different resulting default frequencies?
Prefer set into text rather than look like an input you cannot use. Yes remove the checkbox as we don't currently worry about it..
I figured out 2 ways of getting the current/actual frequency value.
- Set the value when a connection was established and frequency was set in pi-gpiod.js (needs to be deployed at least once)
- Create an extra connection in pi-gpiod.html to only get the current frequency of the selected pin (needs ugly logic for when trying to connect, getting value etc.)
What would you prefer?
Hmm tricky... having 2nd thoughts. Neither ideal. How about - send to node.status on instantiation/deploy of the node (in PWM mode) - but then let it get overwritten by and incoming msg. on Input event.
I'm implementing it with the node.status now, but had troubles with get_PWM_frequency(). I need to investigate on that first. It seems that the callback isn't ever called. I do not have access to a Pi until next sunday, so it will take a little bit until i finish this.
Seems that the callback isn't ever called because there's another command in progress (set_mode() or set_PWM_frequency()). Got it to work so far by adding callbacks for these functions in js-gpio. Need to figure out why this happens. Maybe a restriction of pigpiod socket interface?
Hi did you mean to update this quite so much ? We aren't going to merge 179 file changes... can you flatten it so only the relevant changes are in the PR ? (or close and re-submit)... Thanks
Hi, no this wasn't intentional. Just wanted to rebase with master branch to get other changes. This somehow went wrong :confused: Sorry. As https://github.com/miketrebilcock/js-pigpio/issues/3 is still not resolved i can't finish this PR. Thinking about to change the underlying library, but i will open a discussion for this in the forum.