AllStarLink / app_rpt

Refactoring and upgrade of AllStarLink's app_rpt, etc.
3 stars 2 forks source link

Template-ize simpleusb.conf and usbradio.conf #327

Closed Allan-N closed 1 month ago

Allan-N commented 1 month ago

In the past, configuring more than one SimpleUSB (or USBRadio) node on a system required duplication of large chunks of settings in the "simpleusb.conf" (and "usbradio.conf") file. Managing these configuraitons in the menu(s) was also messy. Updating the files to use templates greatly simplifies our support for multiple nodes.

InterLinked1 commented 1 month ago

Why the change from yes/no to true/false? Most, if not all, Asterisk configs, use the former, not the latter, though I think both are recognized just fine.

I completely agree with changing 1/0 to yes/no.

Allan-N commented 1 month ago

Why the change from yes/no to true/false? Most, if not all, Asterisk configs, use the former, not the latter, though I think both are recognized just fine.

Before the change from 0/1 to false/true I ran the "tune" menu and found the updated simpleusb.conf file contained :

[491300](node-main)
rxboost = false
deemphasis = false
plfilter = true
invertptt = false
preemphasis = false

;;;;; ASL3 Tune settings ;;;;;
devstr = 1-2:1.0
rxmixerset = 500
txmixaset = 500
txmixbset = 500

If the file had been updated with no/yes I would have used those strings. But, since the updated settings were false/true I opted to follow along.

InterLinked1 commented 1 month ago

Why the change from yes/no to true/false? Most, if not all, Asterisk configs, use the former, not the latter, though I think both are recognized just fine.

Before the change from 0/1 to false/true I ran the "tune" menu and found the updated simpleusb.conf file contained :

Pardon my ignorance, but what do you mean exactly by running the tune menu here? I can take a look at that.

Allan-N commented 1 month ago

I used node-setup to exec /usr/sbin/simpleusb-tune-menu, issued "p" to show the settings, and "w" to update the "simpleusb.conf"

InterLinked1 commented 1 month ago

I used node-setup to exec /usr/sbin/simpleusb-tune-menu, issued "p" to show the settings, and "w" to update the "simpleusb.conf"

Looks like this is where that's happening: https://github.com/AllStarLink/app_rpt/blob/master/channels/chan_simpleusb.c#L3187

Based on these results, I propose modifying that line to use yes/no, same with usbradio:

root@debian:/sdb/dev/asterisk/configs# grep -R -e "true" -e "false" | wc -l
66
root@debian:/sdb/dev/asterisk/configs# grep -R -e "yes" -e "no" | wc -l
1936
Allan-N commented 1 month ago

Based on these results, I propose modifying that line to use yes/no, same with usbradio:

Sure. I'll change the false/true's to no/yes's and update chan_simpleusb.c and chan_usbradio.c.

Allan-N commented 1 month ago

Changing the false/true's to no/yes's helped but in testing but I am having an issue with changing some of the other values (e.g. rxboost) from "no" (the default) to "yes" (the new value) and then back to "no". Here, the "yes" that was added is not being removed (since it matches the template) nor is it changed to "no" (the new value).

I am investigating ...

Allan-N commented 1 month ago

NOTE : there's some odd behavior with settings like "rxboost" ... that should be resolved before these changes are merged.

Allan-N commented 1 month ago

File AllStarLink/asl3-asterisk#23 to track the "rxboost" issue. So, this pull request is just to get simpleusb.conf and usbradio.conf template-ized.

@InterLinked1 : can you review?

InterLinked1 commented 1 month ago

File AllStarLink/asl3-asterisk#23 to track the "rxboost" issue. So, this pull request is just to get simpleusb.conf and usbradio.conf template-ized.

@InterLinked1 : can you review?

The samples don't need the template modifications, as they're not intended to provide a functional config, they are mostly for documentation.

Also, the samples should not contain references to ASL3, since that doesn't mean anything in Asterisk, only the rpt configs should.

Allan-N commented 1 month ago

The samples don't need the template modifications, as they're not intended to provide a functional config, they are mostly for documentation.

Is there a problem with the samples being functional? For anyone who wants to start with a fresh configuration it would be nice to copy the .conf files from .../samples to /etc/asterisk. Also, if our menu code expects a templated configuration then providing a sample that is lacking will present support issues.

Also, the samples should not contain references to ASL3, since that doesn't mean anything in Asterisk, only the rpt configs should.

I will remove the ASL3 reference.

Allan-N commented 1 month ago

ASL3 references removed from .conf.sample files.

InterLinked1 commented 1 month ago

The samples don't need the template modifications, as they're not intended to provide a functional config, they are mostly for documentation.

Is there a problem with the samples being functional? For anyone who wants to start with a fresh configuration it would be nice to copy the .conf files from .../samples to /etc/asterisk.

In theory yes, but most sample files aren't like that, and require some configuration as it is. My suggestion is merely in line with how current sample files are, which generally use a minimal config to demonstrate all settings, typically without templates.

Also, if our menu code expects a templated configuration then providing a sample that is lacking will present support issues.

The menu code should work regardless of whether templates are used or not, if that is not the case, I would think that is a bug. The config code in Asterisk (although apparently requiring my 2 patches) work that way.

Also, the samples should not contain references to ASL3, since that doesn't mean anything in Asterisk, only the rpt configs should.

I will remove the ASL3 reference.

Allan-N commented 1 month ago

In theory yes, but most sample files aren't like that, and require some configuration as it is. My suggestion is merely in line with how current sample files are, which generally use a minimal config to demonstrate all settings, typically without templates.

I have de-template-ized the .conf.sample files ... but we (the team) may need to revisit this at a later date.

The menu code should work regardless of whether templates are used or not, if that is not the case, I would think that is a bug.

Sure. But, try to massage these configuration files with bash, sed, awk, ex, etc while handling a mix of configuration files flavors would be much more work. We've had discussions of adopting AMI but that path is presenting its own issues.

The config code in Asterisk (although apparently requiring my 2 patches) work that way.

If we adopt templates for SimpleUSB, USBRadio, and others then, YES, the patches are most certainly needed for "tuning".

InterLinked1 commented 1 month ago

In theory yes, but most sample files aren't like that, and require some configuration as it is. My suggestion is merely in line with how current sample files are, which generally use a minimal config to demonstrate all settings, typically without templates.

I have de-template-ized the .conf.sample files ... but we (the team) may need to revisit this at a later date.

The menu code should work regardless of whether templates are used or not, if that is not the case, I would think that is a bug.

Sure. But, try to massage these configuration files with bash, sed, awk, ex, etc while handling a mix of configuration files flavors would be much more work.

Simply put, using bash, sed, awk, ex, etc are not appropriate ways of updating the Asterisk configs. They make work for the simplest cases (and I do this myself in certain cases), but they are absolutely not intended or suitable for setting up an entire config file or, moreover, modifying existing configs.

We've had discussions of adopting AMI but that path is presenting its own issues.

I'm curious, what are the issues with this approach?

The config code in Asterisk (although apparently requiring my 2 patches) work that way.

If we adopt templates for SimpleUSB, USBRadio, and others then, YES, the patches are most certainly needed for "tuning".

Sounds good - keep me posted on if you notice anything else odd about that or if there are any other related issues that aren't addressed still. If not, I'll go ahead and add that as a patch to PhreakScript in June.

Allan-N commented 1 month ago

Simply put, using bash, sed, awk, ex, etc are not appropriate ways of updating the Asterisk configs. They make work for the simplest cases (and I do this myself in certain cases), but they are absolutely not intended or suitable for setting up an entire config file or, moreover, modifying existing configs.

Agreed. But, one has to look back at what we started with. ASL2 (and prior versions) used shell script driven menus to make the most basic configuration changes. Those scripts were the starting point for what we have right now for ASL3. Are there lots of expectations/assumptions about how the .conf file content? Absolutely. But, if you use the scripts to make most of your changes and other edits don't stray too far then all is well.

I'm curious, what are the issues with this approach?

The biggest issue we found while exploring AMI (so far) was how comments and white space, embedded in the .conf files, were being mangled. As you know, our current .conf files have lots of embedded comments and one AMI update can trash the carefully formatted content.

Is there an AMI path forward? Possibly :-) But, if we do go down this path then we will most likely need to make some big decisions about whether we have in-line comments / documentation in the .conf files. We may also need some changes/fixes to the Asterisk/AMI APIs.

Note: you can find some additional AMI findings in https://github.com/AllStarLink/asl3-menu/blob/develop/php-backend/UpdateConfig-notes.md

Sounds good - keep me posted on if you notice anything else odd about that or if there are any other related issues that aren't addressed still. If not, I'll go ahead and add that as a patch to PhreakScript in June.

We need the PhreakScript and/or the equiv asl3-asterisk patch if we want to template-ize simpleusb.conf (and usbradio.conf) ... so the sooner it's available the better. I would also suggest that this change is dependent on the patch.

InterLinked1 commented 1 month ago

I'm curious, what are the issues with this approach?

The biggest issue we found while exploring AMI (so far) was how comments and white space, embedded in the .conf files, were being mangled. As you know, our current .conf files have lots of embedded comments and one AMI update can trash the carefully formatted content.

Is there an AMI path forward? Possibly :-) But, if we do go down this path then we will most likely need to make some big decisions about whether we have in-line comments / documentation in the .conf files. We may also need some changes/fixes to the Asterisk/AMI APIs.

I think the latter is the real issue, and I'm happy to address those issues after I get more time in a couple weeks. That's the real underlying issue and fixing that would eliminate the barriers to using AMI here and potentially in various other cases as well. Actually, it's not so much AMI as the core config APIs, and as we've seen, the config APIs as they exist now are not perfect

Tim has been documenting some of what he's noticed, here: https://github.com/AllStarLink/asl3-menu/blob/develop/php-backend/UpdateConfig-notes.md

If you've noticed anything else that seems off that isn't listed there, please go ahead and include it there, as I will use that as a basis for testing those issues.

The only downside to AMI I can think of is it required AMI to be enabled to make such changes, but I think other tools already expect that, like website that shows connected nodes and such, so maybe that's not a big deal in practice.

Allan-N commented 1 month ago

I think the latter is the real issue, and I'm happy to address those issues after I get more time in a couple weeks. That's the real underlying issue and fixing that would eliminate the barriers to using AMI here and potentially in various other cases as well.

So far, those have been the biggest issues that I've run into.

Actually, it's not so much AMI as the core config APIs, and as we've seen, the config APIs as they exist now are not perfect

Yes.

Tim has been documenting some of what he's noticed, here: https://github.com/AllStarLink/asl3-menu/blob/develop/php-backend/UpdateConfig-notes.md

If you've noticed anything else that seems off that isn't listed there, please go ahead and include it there, as I will use that as a basis for testing those issues.

Both Tim and I have been contributing to that file. Once we get our "beta" out the door I will restart my efforts on the PHP backend and integrating the backend into the menus. If I run into issues that have not already been called out I will be sure to add them to the notes.

The only downside to AMI I can think of is it required AMI to be enabled to make such changes, but I think other tools already expect that, like website that shows connected nodes and such, so maybe that's not a big deal in practice.

I think we're OK with enabling "local" AMI access. But, there's also been talk of remotely managing a node using AMI. That may present some challenges and concerns.

Allan-N commented 1 month ago

Tim OK'd merging this change.