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

🐛 | gpio_settings.ini moved without notice or doc update #1096

Closed
bbung opened this issue Oct 17, 2020 · 23 comments · Fixed by #1116
Closed

🐛 | gpio_settings.ini moved without notice or doc update #1096

bbung opened this issue Oct 17, 2020 · 23 comments · Fixed by #1116
Labels
Milestone

Comments

@bbung
Copy link

bbung commented Oct 17, 2020

Bug

This commit: f419fb4

Cost me two evenings. Someone moved the gpio_settings.ini and did not update the documentation or added a info on install.sh

What happened

coping and changing gpio_settings.ini in the previous and documented path does not effect anything.

I expected this to happen

Update Doc or add statement to install.sh where to find the config file. or both.

Further information that might help

-->

@bbung bbung added the bug label Oct 17, 2020
@dbffm
Copy link

dbffm commented Oct 17, 2020

I think I encountered the same issue. (fresh buster insallation; 2.1.1 with Spotify and GPIO control)
I changed /settings/gpio_settings.ini and and there was no effect at all. Buttons didnt work as set in the .ini file.

In file /components/gpio_control/gpio_control.py I changed the following line to the exact path of the settings.ini and now buttons are working as they should:
before:
config_path = os.path.expanduser('~/.config/phoniebox/gpio_settings.ini')
after
config_path = os.path.expanduser('home/pi/RPi_jukebox_RFID/settings/gpio_settings.ini')
So somehow this config path is not derived properly.

What is still not working is the rotary encoder; I think it is the same issue than #1095

s-martin added a commit that referenced this issue Oct 17, 2020
s-martin added a commit that referenced this issue Oct 17, 2020
@s-martin s-martin added this to the 2.2 milestone Oct 17, 2020
@s-martin
Copy link
Collaborator

s-martin commented Oct 17, 2020

Sorry for the unnecessary confusion.

When we integrated GPIO control, there was a lengthy discussion about where to put the settings file.

In the long run we want to move all configs out of Phoniebox‘s dir to make upgrades easier (see #797), but I think that should be done at once for all configs.

So for now I adapted in the develop branch all wrong paths to the usual settings dir.

@veloxidSchweiz, @MiczFlor, I hope that’s ok for now.

@RiekeIcke
Copy link

RiekeIcke commented Oct 19, 2020

I just tried a fresh install of the develop branch with on line installer and the scripts stops on line 1162 with the message, that the gpio script was not found.

I would wait for a fix. But as git-noob I was not able to use a previously working (for example v2.0.0):
How can I install v2.0.0 with one line installer? I tried something with alternative url to the install.sh but failed during installation...

I am working with buster-lite, where I was not able to get gpio running. I have two more phonieboxes running perfectly with v2.0 1b6bc25 develop.

@MiczFlor
Copy link
Owner

So for now I adapted in the develop branch all wrong paths to the usual settings dir.
@veloxidSchweiz, @MiczFlor, I hope that’s ok for now.

Yes, that's ok.
Just so I really understand gpio_settings.ini - at first glance it looks like it does never need to be changed. At second glance it looks like it might be changed after the installation according to the hardware setup (e.g. timeBase: 0.1 ;only for RotaryEncoder or functionCallTwoButtons: functionCallVol0 ;only for TwoButtonControl).
In my opinion...

  • if the file is never changed after installation, it should not live inside the settings folder. Where it should live, I don't know yet :)
  • if the file can be changed after installation, the original file should exist with the ending .sample and then be copied during installation to the settings folder (like cp "${jukebox_dir}"/misc/sampleconfigs/startupsound.mp3.sample "${jukebox_dir}"/shared/startupsound.mp3 during the installation)
    • if this is the case, the original file should live in the folder /misc/sampleconfigs/ with the ending .sample

Because files that can be changed and might be changed by the user after installation should not be part of the git code base.

What do you think @s-martin @veloxidSchweiz

@0x0lli
Copy link

0x0lli commented Oct 19, 2020

is there a need to additionally configure /boot/config.txt in regards to using a rotary encoder?

@s-martin
Copy link
Collaborator

is there a need to additionally configure /boot/config.txt in regards to using a rotary encoder?

Not that I’m aware of.

@s-martin
Copy link
Collaborator

@MiczFlor, from my point of view the file will change after installation, because the user will edit the GPIO pins and edit or remove „devices“ in the settings file, depending on the used hardware.

So I agree we should go with the sample mechanism.

@veloxidSchweiz
Copy link
Contributor

Hi,
In principle it should be changed once, in order to 'describe' your hardware configuration. It will not be needed to be changed afterwards. I don't think it is the right mechanism to place it in the 'settings' directory and overwrite it with every installation. That's the reason why it is placed outside, and then should not be part of the git changes.... The ini files are some kind of examples which are describing the common usecases. I do not really see a reason to change the name to .sample

@s-martin
Copy link
Collaborator

.sample is kind of a convention in Phoniebox for sample files, which are copied during installation and then edited by the user.

Huseriato pushed a commit to Huseriato/RPi-Jukebox-RFID that referenced this issue Oct 21, 2020
MiczFlor#1106: Skip WINS configuration in install script when installing Samba
MiczFlor#1104: Media directory is not adjusted in Mopidy, when changed in install script
MiczFlor#1096: gpio_settings.ini moved without notice or doc update
@Huseriato
Copy link

Isn't this something easy to fix in the master? It's just a path that had to be adjusted in a file. I used '../../settings/gpio_settings.ini' in my fork to test this. It's all fine than. I don't understand the problem at all: The installation script checks, if the file exists. If it's not there, than the sample file is copied. If it is there, than the file is untouched. The new installscript now ignores the existing file, because it's in the old directory. Breaking all installations out there. That does not make sense in any way...

@s-martin
Copy link
Collaborator

It's already fixed in the develop branch. We just have some discussions (see above) how to improve the UX more.

We will release (i.e. merge develop to master) this soon.

@Huseriato
Copy link

What config files are moved to ~/.config/phoniebox/ in the next release? I don't like this decision at all. It just looks like cluttering more and more... Because the mopidy config resides in /etc/mopidy and another location I currently don't know. The settings done by the webinterface a in settings/ directory. The chosen installation options are also in settings/. There does not seem to be any migration path for existing users. The install script simply ignores this issue. If you use git pull in RPi-Jukebox-RFID directory the whole configuration is ignored from one to another second. Great.

I'm currently working on a (private) deployment for many non technic affine people to get a phoniebox. This config update is a disaster for this scenario. I'll have to repair many configurations just after delivery.

@s-martin
Copy link
Collaborator

This is still an ongoing discussion, so nothing decided yet.
The advantage would be that for reinstall users often just delete the whole Phoniebox folder, so settings (and music, playlists) are lost.
Separating "program files" from content would somehow solve this.

Of course, if we take this road, we need a clear migration path, so this won't break existing installations.

@Huseriato
Copy link

We will release (i.e. merge develop to master) this soon.

So the ~/.config/phoniebox/ will not be part of the master, correct? And if it will become the desired path, there will be an automatic migration for existing installations?

@s-martin
Copy link
Collaborator

s-martin commented Oct 22, 2020

We will release (i.e. merge develop to master) this soon.

So the ~/.config/phoniebox/ will not be part of the master, correct?

Yes

And if it will become the desired path, there will be an automatic migration for existing installations?

If somebody will implement it :)

@Toqqi
Copy link
Contributor

Toqqi commented Oct 22, 2020

I was struggling to figure out why my buttons wouldn't work.
Everything was reacting fine when testing with simple_button.py and two_button_control.py, but I wouldn't get any reaction in the syslog or from the player.
Thanks @Huseriato for sharing the use of ../../settings/gpio_settings.ini. This fixed it for me.

@Huseriato
Copy link

If somebody will implement it :)

I really think, the migration process should be implemented, before this is changed in master. This had to be in one step. Just my thoughts. ;)

s-martin added a commit to s-martin/RPi-Jukebox-RFID that referenced this issue Oct 23, 2020
* don't version the used gpio_settings.ini file in git
* follow Phoniebox conventions for settings files and store it in misc/sampleconfigs
* Improve documentation
@s-martin s-martin linked a pull request Oct 23, 2020 that will close this issue
@s-martin
Copy link
Collaborator

s-martin commented Oct 23, 2020

So I added PR #1116 to fix all current issues.
@MiczFlor I followed the Phoniebox conventions with .sample files.

@Huseriato and others: a git pull may break existing installations, because I removed settings/gpio_settings.ini form git, so this file may be deleted. Please backup the file, before performing a git pull!

@Huseriato
Copy link

For your information: There are currently many people in Raspberry PI and other forums, that are all struggeling about the same gpio settings bug. Some are not able to get a fresh installation and many of them failed after update. So please offer a migration path for this people. Renaming the file in git can do the trick (gpio_settings.ini -> gpio_settings.ini.sample). The python script can than look for the file and automatically copy the renamed SAMPLE-file to the desired target location.

@s-martin
Copy link
Collaborator

s-martin commented Oct 23, 2020

Currently Phoniebox has no real recommended upgrade path for all users.

Experienced users may use git pull, but for many new users a fresh install (with backup of settings, music and playlists folders) was recommended in the past. This was mainly because git pull would often break exactly things like this. And many regular users may not be able to resolve conflicts on a terminal.

I'm really not sure, how to proceed here.

On one hand we cannot do anything (like backing up the working copy) before a user may git pull. One may argue that you should backup your installation before upgrading, but still a git pull is not supposed to break anything (i.e. removing your current files).
Although if a user has locally modified gpio_settings.ini and uses git pull there's probably still a conflict, if we keep that file under version control. So the git pullupgrade path doesn't work here anyway (that's why I want to remove it from version control in #1116).

On the other hand I'm a little reluctant to keep a gpio_settings.ini.sample or .bak under git version control, because a) this may not be the current configuration the user had edited locally anyway and b) how many years are we supposed to keep it under git version control, because there still may be an old installation out there where one could git pull.

Thoughts, other ideas?

@s-martin
Copy link
Collaborator

I just thought about it and think even without deleting gpio_settings.ini from repo a git pull wouldn’t work, if you changed the the file locally.

Either git pull would override the local settings without any notice or you would have a conflict, which could also destroy your local configuration.

That’s the main reason why @MiczFlor suggested not to version the file.

Considering that keeping the file under version control and removing it could both destroy an existing local configuration I would suggest to remove it now from version control, so at least future installations won’t have this issue.

@Huseriato and others, what do you think?

@Huseriato
Copy link

Huseriato commented Oct 25, 2020

I think the old gpio_settings.ini should be moved - using git - to the sample folder. So a local file (with modifications) is moved to the sample folder. The gpio script can than check, if the target file in the settings/ or ~/.config/phoniebox directory exists or not. If not, than it copies the version from the sample directory to this location. So a modified version of the file is kept and the sample file is also used, if there is no configuration.

@s-martin
Copy link
Collaborator

That’s what PR #1116 does

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

Successfully merging a pull request may close this issue.

9 participants