morrolinux / mpradio

Morrolinux's Pirate radio (PiFmRDS / PiFmAdv implementation with Bluetooth and mp3 support) - Stream music to your car's FM radio or use it as a Bluetooth speaker via headphone jack
GNU General Public License v3.0
107 stars 17 forks source link

Cleanup #33

Closed Hurricos closed 6 years ago

Hurricos commented 6 years ago

Branch may still need some testing, but the changes are minor enough and are mostly installation-related.

Some various cleanup:

XXX: We might want to peg a commit to clone from PiFmRds, in case of future changes. Otherwise, changes in the PiFmRds repository may break this repo.

morrolinux commented 6 years ago

Great job man! This really needed to be done since a while. Just a couple of observations: 1) I don't remember exactly why snd_bcm2835 needed to be blacklisted. I think it caused troubles with pulseaudio (or maybe it still does, even by itself?) because I see you haven't blacklisted it in the new installer. Could you argument a bit the choice? 2) Please note that need2recompile.sh would need to copy the new compiled binary to /usr/local/bin/pi_fm_rds for this to work, or the old executable will still be called, thus no effect would be seen. (It used to work in the past since we were calling it from the sources folder, but I think you already got it :)

Hurricos commented 6 years ago

1) I have not actually removed anything: the effective line to add the blacklist was already commented out. The only thing that wasn't was the removal using sed, but no need to remove if it's never added. Also, having the control being based on the presence of the snd_bcm2835 blacklist was causing issues since it was never being added, so never being grepped. I found 33 copies of blacklist ipv6 in my blacklist.conf... :laughing:

2) Thank you very much for the review. I would never have caught that, I think. Pushing the change.

3) I have been somewhat inconsistent about testing. In this case, I had wanted to install, uninstall, reinstall, then plug in a drive to test out RDS with my father's truck, and then connect to bluetooth to listen in via a portable radio receiver I have. However, last night, I had no access to the truck or physical access to the Pi; I did not want to wake anyone up by trying to get to the office (it is currently being used simultaneously as a CUPS server), so all I did was repeatedly install and uninstall to see what happened (at which point I noticed the holes in the uninstaller). Since I caught and replaced all instances of /home/pi paths being used - there were very few, since the executables tend to be called from only one place each - I figured I was finished.

morrolinux commented 6 years ago

1) lol I haven't thought about that 2) np that's why I check the code before merging :) 3) alright that make sense!!

merging now!