Skip to content
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

Feature to set frequency for pwm output in gpiod node #425

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

PhilippHaefele
Copy link

Feature is optional, so old config should still work.
Depending on sample rate setting in gpiod real values can vary.

@knolleary
Copy link
Member

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.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 66.156% when pulling 26b9588 on Psykoman1990:feature-setfrequency-for-pwm into 84a445d on node-red:master.

@coveralls
Copy link

coveralls commented Mar 30, 2018

Coverage Status

Coverage remained the same at 66.341% when pulling d177d6d on Psykoman1990:feature-setfrequency-for-pwm into 8d45e85 on node-red:master.

@dceejay
Copy link
Member

dceejay commented Mar 30, 2018

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...)

@PhilippHaefele
Copy link
Author

@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?

@knolleary
Copy link
Member

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.

@PhilippHaefele
Copy link
Author

PhilippHaefele commented Mar 30, 2018

@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.

@dceejay
Copy link
Member

dceejay commented Mar 30, 2018

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 ?)

@PhilippHaefele
Copy link
Author

PhilippHaefele commented Mar 30, 2018

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)?

@dceejay
Copy link
Member

dceejay commented Mar 30, 2018

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 ?

@PhilippHaefele
Copy link
Author

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)?

@dceejay
Copy link
Member

dceejay commented Mar 30, 2018

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)

@PhilippHaefele
Copy link
Author

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?

@dceejay
Copy link
Member

dceejay commented Mar 30, 2018

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..

@PhilippHaefele
Copy link
Author

I figured out 2 ways of getting the current/actual frequency value.

  1. Set the value when a connection was established and frequency was set in pi-gpiod.js (needs to be deployed at least once)
  2. 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?

@dceejay
Copy link
Member

dceejay commented Mar 30, 2018

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.

@dceejay
Copy link
Member

dceejay commented Apr 7, 2018

Hi - where did we get to on this ?

@PhilippHaefele
Copy link
Author

PhilippHaefele commented Apr 8, 2018

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.

@PhilippHaefele
Copy link
Author

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?

@dceejay
Copy link
Member

dceejay commented Mar 29, 2019

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

@PhilippHaefele
Copy link
Author

Hi, no this wasn't intentional. Just wanted to rebase with master branch to get other changes. This somehow went wrong 😕 Sorry. As miketrebilcock/js-pigpio#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants