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

Multiple device support #382

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

nicola02nb
Copy link
Contributor

@nicola02nb nicola02nb commented Dec 4, 2024

Changes made

Added multiple device support with command arg -d, --device INDEX
Actions can be executed only to one device at a time

Checklist

  • I adjusted the README (if needed)
  • For new features in HeadsetControl: I discussed it beforehand in Issues or Discussions and adhered to the wiki

@Sapd
Copy link
Owner

Sapd commented Dec 8, 2024

Thank you!

Some points for now:

  • --test-device seems to be completely broken
    • In the end I think it would be nice that also the test device is shown as another device (when the parameter is provided) together with a real device when one its connected
  • -d should be I think a bit more flexible. It should also support -d vendorid:deviceid (like in the --dev menu). However I like the approach with showing the indexes directly in the text when running the program

@nicola02nb
Copy link
Contributor Author

  • --test-device seems to be completely broken

Ok, didn't checked that...

  • In the end I think it would be nice that also the test device is shown as another device (when the parameter is provided) together with a real device when one its connected

You mean to add it to the curernt list of connected devices?

  • -d should be I think a bit more flexible. It should also support -d vendorid:deviceid (like in the --dev menu). However I like the approach with showing the indexes directly in the text when running the program

Maybe an hybrid solution where you can use both of them would be also better...

Question: Which interface I choose to interact with a device changes nothing, right?

@Sapd
Copy link
Owner

Sapd commented Dec 8, 2024

Maybe an hybrid solution where you can use both of them would be also better...

Yes thats what I mean. That both -d index and -d vendorid:deviceid works. There is already a function for that, it just has to be moved to utility.c and modified in a way that also one id works:

static int get_two_ids(char* input, int* id1, int* id2)

You mean to add it to the curernt list of connected devices?

Yes, when the parameter is provided.

Question: Which interface I choose to interact with a device changes nothing, right?

Basically there is no sense in choosing the interface, its already given by the device implementation

@nicola02nb
Copy link
Contributor Author

nicola02nb commented Dec 8, 2024

Running tests locally gives no errors.
I'm wonderig why CI does...

@nicola02nb
Copy link
Contributor Author

nicola02nb commented Dec 8, 2024

Now it should be all done

it just has to be moved to utility.c and modified in a way that also one id works

Missed that...

@nicola02nb nicola02nb force-pushed the multiple-device-support branch 3 times, most recently from 0a7fb6c to dea163b Compare December 10, 2024 14:06
@nicola02nb nicola02nb force-pushed the multiple-device-support branch from dea163b to 855b7df Compare December 10, 2024 15:17
@Sapd
Copy link
Owner

Sapd commented Dec 21, 2024

Will take a deeper look into the PR over the holidays

@Sapd
Copy link
Owner

Sapd commented Jan 14, 2025

One thing still missing/bugs:

./headsetcontrol -o json --test-device

That is to query all devices including features. However it seems to be missing features like battery if deviceid is not specified. For example here it shows the battery of the test device but not of the corsair device

{
  "name": "HeadsetControl",
  "version": "continuous-13-g855b7df-modified",
  "api_version": "1.2",
  "hidapi_version": "0.14.0",
  "device_count": 2,
  "devices": [
    {
      "status": "success",
      "device": "HeadsetControl Test device",
      "vendor": "HeadsetControl",
      "product": "Test device",
      "id_vendor": "0xf00b",
      "id_product": "0xa00c",
      "capabilities": [
        "CAP_SIDETONE",
        "CAP_BATTERY_STATUS",
        "CAP_NOTIFICATION_SOUND",
        "CAP_LIGHTS",
        "CAP_INACTIVE_TIME",
        "CAP_CHATMIX_STATUS",
        "CAP_VOICE_PROMPTS",
        "CAP_ROTATE_TO_MUTE",
        "CAP_EQUALIZER_PRESET",
        "CAP_EQUALIZER",
        "CAP_MICROPHONE_MUTE_LED_BRIGHTNESS",
        "CAP_MICROPHONE_VOLUME",
        "CAP_VOLUME_LIMITER",
        "CAP_BT_WHEN_POWERED_ON",
        "CAP_BT_CALL_VOLUME"
      ],
      "capabilities_str": [
        "sidetone",
        "battery",
        "notification sound",
        "lights",
        "inactive time",
        "chatmix",
        "voice prompts",
        "rotate to mute",
        "equalizer preset",
        "equalizer",
        "microphone mute led brightness",
        "microphone volume",
        "volume limiter",
        "bluetooth when powered on",
        "bluetooth call volume"
      ],
      "battery": {
        "status": "BATTERY_AVAILABLE",
        "level": 42
      },
      "equalizer": {
        "bands": 10,
        "baseline": 0,
        "step": 0.5,
        "min": -10,
        "max": 10
      },
      "equalizer_presets_count": 2,
      "equalizer_presets": {
        "flat": [ 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0 ],
        "random": [ 6.0, -8.0, 1.5, 7.0, -1.0, -7.5, -9.0, 0.0, 9.0, 10.0 ]
      },
      "chatmix": 42
    },
    {
      "status": "success",
      "device": "Corsair Headset Device",
      "vendor": "",
      "product": "",
      "id_vendor": "0x1b1c",
      "id_product": "0x1b27",
      "capabilities": [
        "CAP_SIDETONE",
        "CAP_BATTERY_STATUS",
        "CAP_NOTIFICATION_SOUND",
        "CAP_LIGHTS"
      ],
      "capabilities_str": [
        "sidetone",
        "battery",
        "notification sound",
        "lights"
      ]
    }
  ]
}

@nicola02nb
Copy link
Contributor Author

nicola02nb commented Jan 14, 2025

It Is actually a missing implementation, it displays the values only for the selected device spicefied by -d <index | pId:vId>.
If you want to add it as a feature, that loads battery and other infos, just let me know, and I'll try to add id

@nicola02nb
Copy link
Contributor Author

@Sapd I'm thinking about a solution that if you use -d it displays, loads, and also sends action, only the selected device.... if not, no action will be performed, but all devices infos will be shown.... do you like that?

@Sapd
Copy link
Owner

Sapd commented Jan 28, 2025

Sounds interesting, but what about making it this way: -d behaves exactly like you specified.
But when nothing specified, it will try to query everything? (however I looked into the code might need some effort in main.c)

@nicola02nb
Copy link
Contributor Author

nicola02nb commented Jan 28, 2025

But when nothing specified, it will try to query everything?

I’m not sure about querying all compatible devices. It might unintentionally alter the settings of other connected headphones, if someone has more of them...

behaves exactly like you specified
(however I looked into the code might need some effort in main.c)

Seems doable to me...

@nicola02nb
Copy link
Contributor Author

I'm also thinking of removing the -d index and keeping only -d vendorid:deviceid:

  • It simplfies the implementation
  • Using vendorid:deviceid is faster because I don't need to enumerate all devices when sending actions
  • If someone runs headsetcontrol -d index in a loop the index can change if another compatible device is connected/disconnected

What do you think about that?

@Sapd
Copy link
Owner

Sapd commented Feb 1, 2025

What do you think about that?

Good points! I agree

I’m not sure about querying all compatible devices. It might unintentionally alter the settings of other connected headphones, if someone has more of them...

Why so? its only about querying (and on top only devices which are implemented in HeadsetControl). Basically Im thinking about that in accordance: https://github.com/Sapd/HeadsetControl/wiki/API-%E2%80%90-Building-Applications-on-top-of-HeadsetControl#querying-values

I highly appreciate your work btw (I just need a bit of time to answer currently)

@nicola02nb
Copy link
Contributor Author

nicola02nb commented Feb 2, 2025

I highly appreciate your work btw

I'm glad to hear that :)

its only about querying

Just to get me clear, you mean to query only informations like BATTERY_STATUS and CHATMIX_STATUS, not also the actions.... rignt?

@nicola02nb nicola02nb force-pushed the multiple-device-support branch from 5227a17 to 3eed0e1 Compare February 3, 2025 11:56
@nicola02nb nicola02nb force-pushed the multiple-device-support branch from 3eed0e1 to c92ea25 Compare February 3, 2025 11:58
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.

2 participants