MycroftAI / enclosure-picroft

Mycroft interface for Raspberry Pi environment
https://mycroft.ai/documentation/picroft
GNU Lesser General Public License v3.0
808 stars 193 forks source link

Confusing scripts and hardcoded device ids #132

Open StuartIanNaylor opened 4 years ago

StuartIanNaylor commented 4 years ago

Got a feeling for mycroft noobs these cause raspbian confusion

{
   "play_wav_cmdline": "aplay -Dhw:0,0 %1",
   "play_mp3_cmdline": "mpg123 -a hw:0,0 %1",
   "enclosure": {
      "platform": "picroft"
   },
   "tts": {
      "mimic": {
         "path": "/home/pi/mycroft-core/mimic/bin/mimic"
      }
   },
   "ipc_path": "/ramdisk/mycroft/ipc/"
}

I myself spent some time hammering with Alsa wondering why is this not working to find hardcoded conf entries. Also

#!/bin/bash
# Use this script to execute audio setup actions
sudo amixer cset numid=3 "1" > /dev/null 2>&1
amixer set PCM 79% > /dev/null 2>&1
amixer set Master 79% > /dev/null 2>&1

Purely a duplication of raspi-config does and alsactl store but again by being a late boot order script it confuses those who are used to raspi methods and the docs they are likely to read.

Shouldn't need hidden hardcoded id's so just remove from script and delete audio-setup.sh as raspi-config, docs and help will often give contary methods, so they just confuse.

fermulator commented 4 years ago

I found this after being recommended to fork part of https://github.com/MycroftAI/mycroft-core/issues/2622 into this enclosure-picroft portion of the project.

Explicitly, I would recommend similar as OP original post. See (2) from proposals I listed out there. I assume that it is still necessary to list out the play_* configurations in /etc/mycroft/mycroft.conf? -- we should just remove the hard coded device ID (at minimum).

OP has suggested to remove entirely (though I'm not sure if that is possible?) @forslund hinted that maybe this was added as a workaround and is now causing issues w/ default configuration/setups....

forslund commented 4 years ago

IMHO those play_* lines should be removed from the config entirely, the issue they worked around isn't present in buster as far as I can tell...

fermulator commented 4 years ago

There was some discussion in mattermost : https://chat.mycroft.ai/community/pl/k48nbo3dztbumewh4kwzz7ocgy , confirming that we should be able to remove the hard coded playback commands (which were workarounds) from the system config; as the defaults DO live in the core: https://github.com/MycroftAI/mycroft-core/blob/dev/mycroft/configuration/mycroft.conf#L53