Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bluealsa-client: a new utiility for invoking D-Bus API messages from the command line. #413

Closed
wants to merge 16 commits into from

Conversation

borine
Copy link
Collaborator

@borine borine commented Feb 3, 2021

This is a simple tool wrapping the org.bluealsa Manager1 and PCM1 D-Bus interfaces. It may be useful for selecting the A2DP codec, viewing properties, or enabling/disabling SoftVolume. The other methods and signal monitoring are also implemented, although possibly less useful in practice.

Copy link
Owner

@arkq arkq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice addition to the project, I like that! :)

PS.
Sorry for not making any progres in other PRs, but currently I'm stuck in the middle of turning internals upside down (a little bit, though). I hope I will finish my work in this month, so it will be possible to merge SCO state model. Then, I will start looking at the BIG PR with multi client support :)

NEWS Outdated Show resolved Hide resolved
doc/bluealsa-cmd.1.rst Outdated Show resolved Hide resolved
utils/cmd/Makefile.am Outdated Show resolved Hide resolved
utils/cmd/cmd.c Outdated Show resolved Hide resolved
utils/cmd/cmd.c Outdated Show resolved Hide resolved
utils/cmd/cmd.c Outdated Show resolved Hide resolved
doc/bluealsa-cmd.1.rst Outdated Show resolved Hide resolved
utils/cmd/cmd.c Outdated Show resolved Hide resolved
utils/cmd/cmd.c Outdated Show resolved Hide resolved
utils/cmd/cmd.c Outdated Show resolved Hide resolved
@borine borine changed the title bluealsa-cmd: a new utiility for invoking D-Bus API messages from the command line. bluealsa-client: a new utiility for invoking D-Bus API messages from the command line. Feb 4, 2021
@arkq
Copy link
Owner

arkq commented Feb 4, 2021

There was an email beep when I was writing a comment regarding "cmd" name :) You was faster than me.

Please, keep it "client" for now, but I've just checked on my PC how many binaries end with "*client" and there are few, e.g. "mosh-client", "rpcclient", "smbclient", which are indeed client apps. BUT there are also apps like "keepassxc-cli" or "wpa_cli" which are command line interfaces for related services (maybe bluealsa- is also more a command line interface than client). And there are also "bluetoothctl", "syslog-ng-ctl", "cec-ctl". And to be fair there is one "bccmd".

So, maybe the name should be "ctl" or "cli", but I don't know which would fit better :D

@borine
Copy link
Collaborator Author

borine commented Feb 4, 2021

So, maybe the name should be "ctl" or "cli", but I don't know which would fit better :D

OK - its easy to change - have a think and I'll go with whatever you decide. Maybe there are others reading this who may have suggestions?

I originally had bluealsa-ctl, influenced by pulseaudio's pactl, but decided that clashed with the ALSA ctl plugin, so then went with bluealsa-cmd, based on pulseaudio's pacmd. I have no strong feelings on this, just whatever you prefer.

@arkq
Copy link
Owner

arkq commented Feb 4, 2021

so then went with bluealsa-cmd, based on pulseaudio's pacmd

Ooo I see, I don't have PA on my box installed so I did not find pacmd. Also I do not have any strong feelings about the name, but once it will be merged with master changing the name would not be nice (from the user point of view). So, I'd like to define it in a way compatible with other similar tools.

Like @borine has said, any suggestions are welcomed :)

utils/client/client.c Outdated Show resolved Hide resolved
@borine
Copy link
Collaborator Author

borine commented Feb 5, 2021

A list of potential names for this utility:

bluealsa-cli
bluealsa-client
bluealsa-cmd
bluealsa-command
bluealsa-control
bluealsa-ctl
bluealsa-dbus
bluealsa-manage
bluealsa-pcm

This reverts commit 0559ace.
The requirement for this code in bluealsa-client was removed by
commit 1d92201 when the monitor command was re-written.
@borine
Copy link
Collaborator Author

borine commented Feb 5, 2021

i propose to squash the commits in this PR once a name for the utility has been chosen - I think that should make a final review easier against a single commit.

I've used size_t instead of ssize_t for returns of read() and write(), so that needs fixing, but other than that I hope we are gettiing close to a final review now. Unless you have any new comments, we just need to choose the name. I'll continue to try to think up more candidates, but am not having any inspired ideas at present.

Copy link
Owner

@arkq arkq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i propose to squash the commits in this PR once a name for the utility has been chosen

Yes, there is no need to squash it now, adding new commits it OK. I will squash it anyway when committing to master.

Unless you have any new comments, we just need to choose the name.

That's the toughest task :D From all of the names you've mentioned I'd go for bluealsa-cli anyway. Here is the reasoning: For now this tool could be named as bluealsa-pcm or even bluealsa-pcm-ctl or bluealsa-pcm-cli or something similar, because it uses PCM interface of BlueALSA service (and Manager but only for listening for PCM-related signals). However, I've got some other methods in the Manager interface which could be used by this tool - interface for internal configurations like forcing mono channel, changing settings for encoders, etc. It is not released in the master, but maybe someday I will polish it enough to be public-ready. So in such a way this tool could be a PCM client and controller, so the name "command line interface" fits best in my opinion.

doc/bluealsa-client.1.rst Outdated Show resolved Hide resolved
utils/client/client.c Outdated Show resolved Hide resolved
utils/client/client.c Outdated Show resolved Hide resolved
utils/client/client.c Outdated Show resolved Hide resolved
utils/client/client.c Outdated Show resolved Hide resolved
utils/client/client.c Outdated Show resolved Hide resolved
utils/client/client.c Outdated Show resolved Hide resolved
utils/client/client.c Outdated Show resolved Hide resolved
utils/client/client.c Outdated Show resolved Hide resolved
utils/client/client.c Outdated Show resolved Hide resolved
@borine
Copy link
Collaborator Author

borine commented Feb 6, 2021

I'll make all the changes you request as soon as I can - have real-life priority right now. I'll also change the name to bluealsa-cli. On the subject of names, have you any thoughts on the choice of command names? For example "softvol" could be "softvolume", or "soft-volume"; and should they all be "get-" and "set-"? or anything else ?

@arkq
Copy link
Owner

arkq commented Feb 6, 2021

I'll make all the changes you request as soon as I can - have real-life priority right now.

No problem. I also have a lot of family-related real-life urgent tasks recently - that's why I had to slow down development of my open-source projects :)

On the subject of names, have you any thoughts on the choice of command names? For example "softvol" could be "softvolume", or "soft-volume"; and should they all be "get-" and "set-"? or anything else ?

I haven't thought about that yet. But since the review of the implementation part is more or less done (from my point of view) it might be a good idea to look at this. And right of the bat I've got some ideas/thoughts (not change requests :D). How about something like this:

  • list-pcms <- that is fine
  • properties <- for such task bluetoothctl uses "info" but properties is also OK, however, "information" might show something beyond property list alone (from the word meaning point of view)
  • get-codecs <- OK, but maybe merge it with properties/info ?
  • select-codec <- to this command (and other) I'd add "help" in case where "value" is missing, i.e. select-codec /path/xxx this will act as get-codecs but with emphasized currently selected codec
  • set-volume <- maybe rename to simply "volume" and add volume listing in case of only path given
  • mute <- show mute state when only path given
  • softvol <- "soft-volume" maybe
  • monitor <- OK
  • open <- OK

Copy link
Owner

@arkq arkq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I think that's it. I think that the user experience right now is OK. Help messages looks nice. After these final few changes, it will be ready to go :)

utils/cli/cli.c Outdated Show resolved Hide resolved
utils/cli/cli.c Outdated Show resolved Hide resolved
utils/cli/cli.c Outdated Show resolved Hide resolved
utils/cli/cli.c Show resolved Hide resolved
@arkq
Copy link
Owner

arkq commented Feb 7, 2021

OK, I will squash these commits and make final minor style changes (mixed tab-spaces indentations, double new lines, etc.) and merge it :)

arkq pushed a commit that referenced this pull request Feb 7, 2021
This tool simplifies access to the BlueALSA service D-Bus API from
the command line.

Closes #413
arkq pushed a commit that referenced this pull request Feb 7, 2021
This tool simplifies access to the BlueALSA service D-Bus API from
the command line.

Closes #413
@arkq arkq closed this in deecd2c Feb 8, 2021
@borine borine deleted the utils branch February 11, 2021 10:40
@borine borine restored the utils branch February 11, 2021 10:40
@borine borine deleted the utils branch February 11, 2021 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants