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

rotary encoder service sample: fix path to rotary_encoder.py #990

Closed
wants to merge 1 commit into from

Conversation

doliverio
Copy link

The path to rotary-encoder.py is outdated in the sample configuration of the service.

old path:
/home/pi/RPi-Jukebox-RFID/scripts/rotary-encoder.py

new path:
/home/pi/RPi-Jukebox-RFID/components/gpio_control/GPIODevices/rotary_encoder.py

@MiczFlor
Copy link
Owner

Hi @doliverio
thanks for catching this issue. I am not sure actually if this is the way it should work, let me get @s-martin into this discussion to ask:
Wouldn't it be better to make a copy of the rotary encode into the scripts directory and there change the PIN for the rotary buttons in the code? Otherwise the changes in the file which is in github would always mean that - when creating a pull request - the changed file would come up in the pull request?

@s-martin
Copy link
Collaborator

s-martin commented May 31, 2020

I’ve just read a little through the /components/gpio_control/ and I think the intention is that install.sh installs a service for gpio_control.py, which controls all the devices including rotary encoder.
Files would stay in the components dir, but the settings file should be located in ~/.config/phoniebox/gpio_settings.ini.

As config takes place in this file, there wouldn’t be any changes for pins in the actual code files, so copying would not be necessary.

The Readme is not up to date yet, so these are just my assumptions.
@veloxid or @veloxidSchweiz, are these correct?

I think we would need to update the wiki or any other docs and integrate install.sh in the default script?

bottom line: I think this PR would not reflect the new behavior.

Copy link
Collaborator

@s-martin s-martin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think that the new implementation of rotary encoder and other GPIO devices would work with this PR

@doliverio
Copy link
Author

@MiczFlor @s-martin Thanks for the explanation. Since this PR would not lead to the desired result and I am not sure how to achieve it, feel free to close this PR

@s-martin
Copy link
Collaborator

Missing integration tracked in #991

@s-martin s-martin closed this May 31, 2020
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.

3 participants