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

Enhanced Encoder support and new GPIO Pinout to enchance compatibility with I2S DAC #350

Merged
merged 6 commits into from
Dec 10, 2018

Conversation

martinclausen2
Copy link

First of all thanks for the nice project!
I did a little work on the code to optime it for my needs. Please see yourself if there is anything useful for the community in the changes / fixes / new features.
Disabling the buttons with the same functionality as the encoders might not work for everyone. Maybe we can find a solution to persist GPIO assignment elsewhere to avoid setting them up each time a new version is deployed.

I2S DACs and GPIO
Change: Current GPIO assignment will block the use of external DACs with I2S interface. Therefore I suggest the reorganization of the GPIOs.

Rotary Encoder Support
New feature: Two encoders, one for volume, second for track (disabled GPIO buttons for track and volume)
Fix: added missing user, group and path to python for execution to sample systemd service configuration
Changes: Switches and switches in encoders are not clearly separated. I suggest to handle encoders in the encoders service and buttons in the buttons service exclusively.
Additionally I suggest to enable pull-ups on the encoder inputs to allow for encoders without an PCB containing pull-ups.

Status: changes are verified on a working system with 1.1.8.RC2

MiczFlor and others added 4 commits November 25, 2018 17:47
Analytics script to post with bug reports
remove switch functionality from encoder program, since is covered by gpio-buttons.py
added second rotary encoder channel for tracks
adjusted GPIO pins
disable GPIOs which functions are covered by rotary encoders
Copy link
Contributor

@marcohorstmann marcohorstmann left a comment

Choose a reason for hiding this comment

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

Please upload this as separate e.g. gpio-buttons.py.roteryencoder.sample This will prevent to break it for all other how doesn't use rotery encoder. Maybe we should build a "setup script" to prevent more and more gpio-buttons.py.examples and nobody will get it. Would this help?

Which pins are blocked by the I2S DAC?

I work already on a prototype setup script.

@martinclausen2
Copy link
Author

martinclausen2 commented Nov 27, 2018

Good suggestion. Please have a look at the new code.

I think the setup script is a great idea. I had the impression may are confused by the options including myself (thinking I killed my DAC or my Pi or both ...). It also eases deployment, if no one has to fiddle around with the code anymore. Would you directly write code from the script or modify a configuration file? I would vote for the later.

Generally all the DACs e.g. ebay ES9023, PCM5102A, ... or assemblies from Hifiberry or Justboom require GPIO 19, 21and 18 which is the fixed I2S interface of the processor. Hifiberry requires additionally GPIO 20 because the DAC do not have their own clock source and I2C for the HAT configuration. Depending on the exact model additional pins might be required.
(see https://www.hifiberry.com/build/documentation/gpio-usage-of-hifiberry-boards/ for details)
I would also recommend for keep the SPI interface free for a RFID reader. Here we are fixed to GPIO 8, 11, 10, 9 for SPI and one additional GPIO for reset, I used GPIO25.
Start / Shutdown always is on GPIO 3 to be able to wake the Pi up again.

Fix voltage on RPi side for encoder KY040 connection
Provide separate example of encoder compatible gpio button script

Revert "make GPIO selection compatible with directly via I2S connected DACs"
This reverts commit e03e4c7.
@marcohorstmann
Copy link
Contributor

My prototype is like a questionnaire. and has not much logic inside of it. I want to write code from the script and not modify a config file. It work better for me. Normally you setup one the hardware and not change it anymore later.

You can find it in my forked branch with my first prototype here:
https://github.com/marcohorstmann/RPi-Jukebox-RFID/blob/gpiostupscript/scripts/installscripts/setup-hardwarebuttons.py
Actually I work on generating the gpio-buttons.py which also includes the rotary encoder script to generate only script and service for this. Later I will try to buildin the "default or suggested" PINs into
the system.

BTW the Hifiberry is using depending which product serveral gpio. I'm not sure if we will find a default setup for everyone.

@martinclausen2
Copy link
Author

I had a look at it and I think it would be of great help.
But would one have to go through the questionnaire each time the system is upgraded to a new phoniebox version? That would be for me the reason to use a config file.
Thanks for the advice with the Hifiberry, I extended my comment, that the GPIOs I named previously are only the minimum set and other might be required depending on the model.

@marcohorstmann
Copy link
Contributor

@martinclausen2 No only to setup once the hardware buttons. the resulting gpio-buttons.py will not be changed while updating phoniebox code. You only need to change it when e.g. commands to the playout_controls.sh will be changed or the playout controls will changed completely. Both I think are
so massiv changes that it will not happen very often maybe never.

@marcohorstmann
Copy link
Contributor

@martinclausen2 Take a look again. Just created the basic file creation logic. I need to think about how we can check that a user are not using the same number of multiple things.

@martinclausen2
Copy link
Author

Ok, convinced.

I would try the following approach:

  • For each hardware a configuration containing the required pins is stored
  • A configuration belongs to a group like buttons, encoders, DAC, RFID, reader, etc, the user can select only one item out of each group.
  • The user selects the combination of configurations and the algorithm sums up the counts for each gpio from all configurations. If a count above one appears, the configuration is invalid. We would need however a special treatment for shared pins like SCL and SDA from the I2C bus.

It seems to me we need a compiled overview of what hardware needs which functionality. Maybe I can start that in the wiki.

I wonder if it would be easier to maintain, if we had a generic code generator script that would process some kind of pre-python script and insert the selected configurations. I found the code somewhat hard to read. But may that adds to much complexity.

@MiczFlor
Copy link
Owner

Hi @marcohorstmann @martinclausen2
in my experience a code generator script is the kind of idea that works great for the person who wrote it and generates uncountable questions, requests and bug reports from those who then want to use it. It's a great idea, don't get me wrong, but it is a software project that needs a maintainer. (believe me, I know).
I think it would be much better if we document solutions in the wiki. As I said in another thread, I would like to support the USB only version as "core" and everything else, by nature of this project, is experimental. And from what I read in many posts here, a few people are actually very happy about what they learned about command line and linux because they jumped into the deep end of the Phoniebox pool and learned to swim more easily than they thought.
So my approach would be: excellent documentation beats specialised code.
What do you think?

@martinclausen2
Copy link
Author

Hi @MiczFlor I guess documentation is the starting point anyway. I never build a code generator myself, so I am more then happy to take advice there. I just had the impression that users need more guidance to go beyond the core version. I will try to spend some time on a documentation about which hardware uses which resources. We can still then go one or the other way.

@MiczFlor
Copy link
Owner

MiczFlor commented Dec 2, 2018

Hi @marcohorstmann @martinclausen2
Since I can not test this code, because I do not have the hardware. And following the "never change a running system" approach (well, it's never running for everybody, but currently, the code base seems to generate fairly few complains :) and because there was this branch opened about "provide a stable version, please", I am hesitant to merge changes. But I do not want to lose this work either. What's the best way to use this in the code base?

@martinclausen2
Copy link
Author

@MiczFlor @marcohorstmann The newer commit should not break the code for most users anymore. I am also happy to take care of it.
Could we define for the future some kind of stable interface between extra hardware and the proposed core project? Users could use a script like the one proposed by @marcohorstmann to inject then the extra code and the core code basis would stay untouched.
@MiczFlor I am happy to donate hardware, if that is of any value for you.

@martinclausen2
Copy link
Author

@marcohorstmann Would you like to review this wiki page: Hardware Pinout Overview

@marcohorstmann
Copy link
Contributor

@martinclausen2 later this week. I‘m this week at a business trip and no time for crosscheck this. First impression is good

@Franzformator
Copy link
Contributor

Franzformator commented Dec 4, 2018

If you are rearranging the GPIO Pins I would appreciate to introduce the GPIO Pin for "Play only if RFID is near the reader" ( #62 or #43 ) to the GPIO Script.
I love this feature and I think it should be unified for all user. At least as place holder for later improvements.

@Franzformator
Copy link
Contributor

Maybe, it would also support other users to have a place holder example in the button script for the Playlist-Button function (#340).
I think this is also a feature that worth to be continued and improved.

@martinclausen2
Copy link
Author

Added a proposed button, encoder, amp GPIO layout - yet without the proposals of @Franzformator - to the wiki. Would a new layout something we could align on?

@MiczFlor
Copy link
Owner

MiczFlor commented Dec 9, 2018

Hi @martinclausen2
great work. Could @Franzformator and @marcohorstmann give this the "thumbs up", because I have not GPIO :)

@marcohorstmann
Copy link
Contributor

As far as I could see. It looks good. Good work @martinclausen2.
@Franzformator Adding this additional reserved GPIO makes sense.
An assignment for #62 oder #43 sounds good.
Including an playlist button example is an good idea, BUT no reserved GPIO for this.
If you want this feature you also maybe need several playlist buttons.

@Franzformator
Copy link
Contributor

Thank You @martinclausen2 !!

Could you please add the "Play only if RFID is near the reader" GPIO - at least to the classic Pinout. This function needs only one Pin and I think several users will use it.

I think we should put the "Playlist-Button" to a Wiki section with implementation examples. For this function you need several Pins, so collisions are inevitable.

@martinclausen2
Copy link
Author

Thank you all for your feedback. I added the "Play only if near" GPIO and recommendations for the playlist buttons GPIOs. I also aligned my pull request with the documentation to generate the least breaking changes.

@MiczFlor
Copy link
Owner

Hi @martinclausen2 @marcohorstmann @Franzformator
thanks for the input, discussion and contribution. It's going into master tonight :)

@MiczFlor MiczFlor merged commit f0b411a into MiczFlor:develop Dec 10, 2018
@MiczFlor
Copy link
Owner

Hi @martinclausen2 @marcohorstmann @Franzformator
one last question, even though this is already closed. In the wiki there is also this page:
https://github.com/MiczFlor/RPi-Jukebox-RFID/wiki/Using-GPIO-hardware-buttons
the new one from @martinclausen2 is this page:
https://github.com/MiczFlor/RPi-Jukebox-RFID/wiki/Hardware-Pinout-Overview
Should they be merged into one? Should one be deleted? Or is this good as it is and might only need to live in the same section in the sidebar?

@martinclausen2
Copy link
Author

Hi @MiczFlor @marcohorstmann @Franzformator I would vote for a link. Overview on the pinout page, links on the specialized pages. I added links to all relevant pages. I hope you like it.
Should we at a certain time point switch from the classic to the flexible layout? This involves breaking changes, but I think it helps new users. So there is value enough in it to allow for breaking changes.

This pull request was closed.
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