search

AllStarLink / app_rpt

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

SimpleUsb CLI menu options not present #256

Closed encbar5 closed 6 months ago

encbar5 commented 6 months ago

There are functions in the simpleusb source code that do not work from the CLI.

channels/chan_simpleusb.c:2773 seems to indicate that "susb tune menu-support" is a valid command, but from the CLI I only get: No such command 'susb tune menu-support'

channels/chan_simpleusb.c:2759 seems to indicate that "susb tune" will display current settings, but from the CLI I only get: No such command 'susb tune'

KB4MDD commented 6 months ago

I believe there are some commands that are intended to be used by the external program simpleusb-tune-menu.

I will double check on susb tune with no parameters. I know pressing the tab you see the full commands. It is possible that recent changes to show all of the commands broke something.

InterLinked1 commented 6 months ago

I believe there are some commands that are intended to be used by the external program simpleusb-tune-menu.

I will double check on susb tune with no parameters. I know pressing the tab you see the full commands. It is possible that recent changes to show all of the commands broke something.

Part of the issue is this:

e->command = "susb tune {rx|rxd|txa|txb|flash|swap|load|save|nocap|rxcap|txcap|menu-support}";

This registers the command as requiring susb tune followed by one of those terms, so susb tune on its own will never be recognized to be a valid command.

I don't know if there is an Asterisk mechanism to do that while making those terms optional. It might be better and more semantic to add a term explicitly to show settings. Personally I do not like the idea of valid commands prefixing other valid commands. I think that would be highly frowned upon. @KB4MDD do you have an idea for a more appropriate name, maybe susb show settings or something of that sort?

tsawyer commented 6 months ago

There is no such command as susb tune menu-support. What you are seeing in the source may be something to do with the CLI simple-tune-menu menu program which is used not at *CLI> prompt but at the Linux CLI. simple-tune-menu is generally the way USB is set up and it's generally called from a Wiptail ncurses menu.

Nothing appears to be wrong with the susb commands on the (from debs) system I am running.

There is code to support both susb tune and susb tune menu-support, but it's essentially "unreachable" right now, which is why I think they appear not to exist.

I am not sure if these were added recently, but at least per the blame it was there before the last refactor.

encbar5 commented 6 months ago

Yes, exactly, there is a susb tune and susb tune menu-support command in the source code, you just can't reach them from the Asterisk CLI. So I guess my question is, is this on purpose? I could be wrong but I don't think it is. The dump of settings that is supposed to happen on susb tune seems like it would be useful to have, maybe under another command like Naveen suggests.

As for the menu-support, I guess I figured if it doesn't work on the CLI, it wouldn't work as an interface to the external programs, but I could be wrong here - I haven't tried it. Has someone tested these?

tsawyer commented 6 months ago

As for the menu-support, I guess I figured if it doesn't work on the CLI, it wouldn't work as an interface to the external programs, but I could be wrong here - I haven't tried it. Has someone tested these?

Yes, simple-tune-menu won't work if susb doesn't. I believe simple-tune-menu is just doing asterisk -rx "susb ...".

Both the susb commands and simple-tune-menu work when Asterisk is built with phreaknet script as of my last testing a couple weeks ago.

encbar5 commented 6 months ago

@tsawyer Are you saying this is not an app_rpt issue? It should be an asl3-asterisk issue because of the packaging?

I'm not sure how to understand that it's the fault of the debian packaging, unless @InterLinked1 can help me understand how it would even be possible to cause this issue, like with different menuselect options before build?

InterLinked1 commented 6 months ago

@tsawyer Are you saying this is not an app_rpt issue? It should be an asl3-asterisk issue because of the packaging?

I'm not sure how to understand that it's the fault of the debian packaging, unless @InterLinked1 can help me understand how it would even be possible to cause this issue, like with different menuselect options before build?

I think this is an app_rpt issue, I agree I don't see how packaging would be relevant. CLI command behavior will be the same regardless.

It shouldn't be difficult to fix this, somebody needs to decide how the commands should be named and what commands should actually exist and then we can make it so.

tsawyer commented 6 months ago

What I know is there wasn't any app_rpt problems a couple of weeks ago when @KB4MDD and I throughly tested the usb stuff. I'm running on the same hardware now but simpleUSB working with the deb version. It's possible there was a regression.

The simpleusb command names are consistent as far as I can see. I'm not sure what the "menu-support" string @encbar5 found when inspecting the code. That's worth investigating. When @KB4MDD comes up for air I'm sure he'll be happy to look into it as he has taken the lead on those channel drivers. Let's see what he has to say.

KB4MDD commented 6 months ago

I have corrected the menu-support issue by adding it as a valid command. The makes simpleusb-tune-menu and usbradio-tune-menu work properly. I missed this during testing.

The only issue that remains is issuing the command susb tune with no parameters. As @InterLinked1 pointed out the mechanism to show the susb tune commands with the tab key restricts the command to those that are instructed in the code. Blank is not a valid option.

We have two options: 1) Add susb tune settings - which would show the settings and easy to implement. If the user attempted to use susb tune, as they have in the past, it would show all of the commands on the command line. or 2) Add a new command susb show settings.

Option 1 is the simplest to implement. Option 2 would require adding a new command, which is not alot of code.

@tsawyer and @InterLinked1 can you suggest a direction.

InterLinked1 commented 6 months ago

I have corrected the menu-support issue by adding it as a valid command. The makes simpleusb-tune-menu and usbradio-tune-menu work properly. I missed this during testing.

The only issue that remains is issuing the command susb tune with no parameters. As @InterLinked1 pointed out the mechanism to show the susb tune commands with the tab key restricts the command to those that are instructed in the code. Blank is not a valid option.

We have two options:

  1. Add susb tune settings - which would show the settings and easy to implement. If the user attempted to use susb tune, as they have in the past, it would show all of the commands on the command line. or
  2. Add a new command susb show settings.

Option 1 is the simplest to implement. Option 2 would require adding a new command, which is not alot of code.

@tsawyer and @InterLinked1 can you suggest a direction.

If @tsawyer has a preference we can go with that.

My take as an Asterisk user is that CLI commands often use the form "subsystem verb noun" e.g. "core show settings", or "dialplan reload", etc. so "susb show settings" is more semantic, since we're showing settings, not tuning settings.

tsawyer commented 6 months ago

My take as an Asterisk user is that CLI commands often use the form "subsystem verb noun" e.g. "core show settings", or "dialplan reload", etc. so "susb show settings" is more semantic, since we're showing settings, not tuning settings.

Sounds good.