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

HID host optionally disable fetching report descriptor #635

Open
Slion opened this issue Oct 15, 2024 · 4 comments
Open

HID host optionally disable fetching report descriptor #635

Slion opened this issue Oct 15, 2024 · 4 comments

Comments

@Slion
Copy link

Slion commented Oct 15, 2024

Is your feature request related to a problem? Please describe.
Especially in the context of multiple HID connections the storage of report descriptors in a single buffer is problematic.
Say if you want up to 8 connections you should have like a 4KB buffer with each device having up to a 512KB descriptor.
Then your app still need to copy those descriptors anyway as they are moved around the buffer when devices a disconnecting.
See: #633 (comment)
On top of that the app may need to do the exact same SDP query to fetch other attributes anyway.
So being able to disable that SDP query would prevent a redundant query, see: #618

Adding documentations to hid_host_init would be great too.

Describe the solution you'd like
Do not fetch report descriptor when hid_host_init is passed a NULL pointer and/or a size of zero.

Describe alternatives you've considered
None

Possible solution

static void hid_host_handle_start_sdp_client_query(void * context){
    UNUSED(context);
    btstack_linked_list_iterator_t it;    
    btstack_linked_list_iterator_init(&it, &hid_host_connections);

    while (btstack_linked_list_iterator_has_next(&it)){
        hid_host_connection_t * connection = (hid_host_connection_t *)btstack_linked_list_iterator_next(&it);

        switch (connection->state){
            case HID_HOST_W2_SEND_SDP_QUERY:
                connection->state = HID_HOST_W4_SDP_QUERY_RESULT;
                connection->hid_descriptor_status = ERROR_CODE_UNSUPPORTED_FEATURE_OR_PARAMETER_VALUE;
                break;
            default:
                continue;
        }
        /* Don't fetch report descriptor if no storage for it */
        if (hid_host_descriptor_storage_len==0 || hid_host_descriptor_storage=NULL){
            return;
        }
        /* Issue SDP query to fetch report descriptor */
        hid_descriptor_storage_init(connection);
        hid_host_sdp_context_control_cid = connection->hid_cid;
        sdp_client_query_uuid16(&hid_host_handle_sdp_client_query_result, (uint8_t *) connection->remote_addr, BLUETOOTH_SERVICE_CLASS_HUMAN_INTERFACE_DEVICE_SERVICE);
        return;
    }
}
@Slion
Copy link
Author

Slion commented Jan 21, 2025

HID report descriptor is wasting even more memory if you want to support both BLE and Classic devices you also need to provide BLE a large 4K buffer through hids_client_init. This design really does not scale well as far as memory consumption goes.

Adding that extra 4K buffer actually tripped me over on Pico W got OOM panic. I'm planning to switch to Pico 2 W and it should be fine there. Still we should really come up with a design that does not require such large static buffers that we still need to copy anyway.

@mringwal
Copy link
Member

Thanks for the suggestion. Not fetching the HID Descriptor would be easy to implement, as in your suggestion. However, HID Host still needs to query SDP to get the L2CAP PSM and a few other values.

I'd like to better understand your reasoning. Why do you want/need to store the HID descriptor in the application?
The idea had been to access the one stored in HID Host with with hid_descriptor_storage_get_descriptor_len() and hid_descriptor_storage_get_descriptor_data()

@Slion
Copy link
Author

Slion commented Jan 23, 2025

Why do you want/need to store the HID descriptor in the application?

The app persists HID report descriptors of paired devices. It actually needs them before devices are connected.
N devices can be paired with the app, currently set to 8 but 16 would be nice to have.
Can't recall what is the max size of an HID report descriptor but I have a device here with 319 bytes.
So if your take 512 bytes as your max HID report descriptor and want to connect 8 devices you need a 4K buffer that's mostly never going to be used in full as you would seldom connect all devices at once and most devices have much, much smaller descriptors anyway.

Maybe the way forward is for the app to provide a buffer large enough for one descriptor which is only valid for the app to copy from a callback and will then be overwritten next time a device connects. That or just let the app fetch the descriptor.

@mringwal
Copy link
Member

Hi. I've made a small commit on develop branch which allows to pass NULL for the HID Descriptor Storage. It will still do the SDP query, but ignore the HID Descriptor. Could you check if this already helps with your project?

Now.. you still need the HID Descriptor in general and are also interested in getting other SDP Record values.
One option would be to allow the app to register it's own SDP query callback and call that from the HID Host one. I guess that would allow for a single SDP query without much additional logic. Next, if you already cache the HID Descriptor, HID Host could do an SDP query for only the required attributes. As required is then the union of the attributes required by BTstack and the app, I could foresee a way for the app to provide it's own des_attribute_id_list. What do you think about these options/ideas?

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

No branches or pull requests

2 participants