Add Custom config.txt copy to boot loader during update and version check

[ziggimon]

We noticed that our custom config.txt isn't being copied from the system image to the bootloader on updates.

This should allow for a string in the config.txt file to be matched and version checked. So if custom version on system image is higher than on the bootloader it will update it.

Thoughts are welcome.

[HiassofT]

This looks really dangerous to me. If the user has modified config.txt (eg added codec licence keys) these changes will be silently dropped. Dont't do that!

At the very least you should check if config.txt was modified.. Replacing a stock config.txt with an updated version could be somewhat acceptable, but still I'm not sure if I'd want to have that happening on my systems.

What are your reasons for wanting to change config.txt?

[ziggimon]

Our goal was to add primarily dtparam option and possibly clock tweaks. And I completely follow the reasoning of not overwriting they key or other custom settings. I'll look at reworking this to not overwrite the entire file.

[ziggimon]

But primarily we noticed that when we added an updated config.txt to our release it wasn't replacing the existing config.txt on the device, so we wanted to add our own config.txt that gets installed via: https://github.com/libreelec/LibreELEC.tv/blob/master/packages/tools/bcm2835-bootloader/package.mk#L53

[HiassofT]

dtparam/dtoverlay should now also be possible via command-line commands - but I haven't tested that yet myself.

Other than that doing config.txt changes in a kodi addon might be the better solution. Then you could give users more choice about which settings to apply etc.

[ziggimon]

We are not using kodi.. See where the PR originates. For our build dtparam is default on to allow for audio on the RPi, so for us we need it default on. No need for the user to fiddle with dtparam settings.

[HiassofT]

OK, now I noticed the "plexinc" - I have to add that I'm not familiar with it at all :-)

I'd still say doing changes during the update process is the wrong place, the user should have some control over that.

People might be using I2S soundcards on the RPi so silently adding dtparam=audio=on might not be the right choice. Clock tweaking can also be dangerous.

If you want to do post-update (distro-specific) stuff I think it might be best to move that code to a separate post-update-hook script - for example add something like this to the end of update.sh (just before the sync):

[ -f $SYSTEM_ROOT/usr/share/bootloader/post-update-hook.sh ] && sh $SYSTEM_ROOT/usr/share/bootloader/post-update-hook.sh

[ziggimon]

That seems more sane. I think I'll have a ponder about how to do it right, though adding a post-update-hook.sh means we will have to add another tweak to add the script. We are really trying not to having downstream changes to the LE base tree in order to allow for other distro's.

Thanks for input.

[MilhouseVH]

Overwriting an existing config.txt is not a good idea for the reasons stated, so this PR as it stands won't be accepted. However if you must modify config.txt then a hook script sounds a better idea - this could be distro-specific and as complicated as you like but leaves mainline LE pretty much unchanged.

[MilhouseVH]

You could also consider putting your Plex specific settings in a secondary config file using the directive include distroconfig.txt added to config.txt - see raspberrypi/firmware#414. With a suitable "Do not edit" warning in distroconfig.txt you could then overwrite this file relatively safely.

It would just need distroconfig.txt copied to the image (if it exists in $DISTRO/blah), and then added to update.sh where it would always be copied over to the device if it exists in the image.

[awiouy]

Hello,

I was playing around with the dtoverlay command line. It works fine, but its behaviour is peculiar, eg dtoverlay dtparam audio=on will load on top of previously loaded dtparam dtoverlays, and therefore will only change the effect of audio (to on) but not change the effect of other dtparams, eg LEDs

In summary, you can turn ALSA on/off on demand with the dtoverlay dtparam audio=on/off command, if you have the opportunity to run that command. If you intend to do that often, it may be worth considering removing earlier dtoverlay dtparam audio= overlays, see the dtoverlay -h command (needs extra logic).

Maybe this is worth a little shell which turns audio on, if aplay -l does not find devices. I could do that before tomorrow.

[ziggimon]

@MilhouseVH I actually like that idea about adding an included config. That would allow us to test out changes internally before shipping them and ensuring that we don't overwrite the keys users add to the config.txt. I think refactoring to do it that way may make more sense. I'll take a look at doing it that way.

[MilhouseVH]

I'll take a look at doing it that way.

Yes, I agree.

Some things to be aware of: Conditional Filters (see "CONDITIONAL FILTERS" section here, and also issues raspberrypi/firmware#320 and raspberrypi/firmware#593, in particular comment raspberrypi/firmware#320 (comment) relating to the behaviour of [all]).

For example, if the user adds the following to the beginning of config.txt:

[pi1]
arm_freq=1000
gpu_mem=128

[pi2]
arm_freq=1100
gpu_mem=320

[pi3]
arm_freq=1400
gpu_mem=320

then, if you have the following at the end of the config.txt

include distroconfig.txt

your distroconfig.txt file will only be included when the SD card is in a Pi3 - it won't be included for a Pi1 or Pi2.

In order that your include is always loaded, regardless of filter, then having the following at the end of config.txt should work:

[all]
include include distroconfig.txt

I'm also assuming the config loaded last overrides any other values - if you include your distroconfig.txt file at the beginning of config.txt then potentially any values you set could be overridden by the user within config.txt (which may or may not be desirable).

[ziggimon]

I love those conditionals, was not aware of those.. We just won in yatzee or something.. We have been looking at a way to do conditional frequencies for rpi2/3 as we have some gpu tweaks that help our UI be smoother. Thanks!

[ziggimon]

So I changed it to use a distroconfig.txt file. Curious on feedback on this.

[MilhouseVH]

Not entirely sure about this one. I think you should be adding this to your own DISTRO specific config.txt. However, if the include causes no error if it doesn't exist, then maybe there's no harm (and means a distroconfig.txt could be added later, if required). Worst case, we add a "common" distroconfig.txt that is blank?

[ziggimon]

I can see the logic in that, however if a user moved from upstream LE to a custom distribution then the overrides would require a full clean install. Thats why I pitched having it in the upstream config file. That way we could avoid shipping our own config.txt and only ship a distroconfig.txt file in the case we wanted to override the upstream version.

[ziggimon]

Made some suggested changes. Input most welcome.

[MilhouseVH]

Almost there! :)

Can you make it cp -p like all the other copies in this file. Also, you need to rebase this file after #437.

[ziggimon]

Should be rebased now.

[popcornmix]

Not directly related to this PR, but I think the base config.txt needs updating:
decode_DTS / decode_DDP haven't been supported for years and should be removed
The overclock suggestions are Pi 1 specific and would underclock Pi2/Pi3 so should be removed/updated.
The overscan settings only affect the splash screen (using a console framebuffer) and have no effect on Kodi, so should probably be removed as more likely to cause confusion than help.

If you want something useful in there, then some commented out examples of overlays for common I2S sound cards and GPIO LIRC would be good. Details here.

[MilhouseVH]

I'll take a look at it, thanks @popcornmix.