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

Add callback for audio volume range (uac2 only) #275

Closed

Conversation

electretmike
Copy link
Contributor

@electretmike electretmike commented Nov 21, 2024

Summary by CodeRabbit

  • New Features

    • Introduced a new function for retrieving a volume range table, enhancing audio control capabilities.
    • Updated volume control request handling for improved modularity and functionality.
  • Bug Fixes

    • Adjusted logic for handling request types to ensure correct operations during volume control.

Copy link

coderabbitai bot commented Nov 21, 2024

Walkthrough

The changes introduce a new weak function, usbd_audio_get_volume_table, in the class/audio/usbd_audio.c file, which populates a volume table with predefined values. This function is utilized in the handling of the AUDIO_REQUEST_RANGE case to replace direct data buffer assignments. Additionally, the handling of AUDIO_REQUEST_CUR has been updated to check the request type direction for volume control. The header file class/audio/usbd_audio.h has been modified to declare the new function, enhancing the audio interface driver's functionality.

Changes

File Change Summary
class/audio/usbd_audio.c Added weak function usbd_audio_get_volume_table for volume table retrieval; modified AUDIO_REQUEST_RANGE handling to call this function; updated AUDIO_REQUEST_CUR handling for request type direction.
class/audio/usbd_audio.h Added function declaration for usbd_audio_get_volume_table.

Poem

In the code where volumes play,
A new table joins the fray.
With requests now clear and bright,
Mute and set, all done just right.
Hops of joy in every line,
Audio bliss, oh how divine! 🐰🎶


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (1)
class/audio/usbd_audio.h (1)

32-32: Add documentation for the new function.

The function declaration looks good and follows the existing patterns. However, it would be beneficial to add documentation that describes:

  • Purpose of the function
  • Parameters and their valid ranges
  • Ownership/lifetime of the volume table
  • Whether the table is read-only or modifiable

Consider adding a documentation block like this:

+/**
+ * Get the volume table for a specific bus and endpoint
+ *
+ * @param busid: USB bus identifier
+ * @param ep: Endpoint number
+ * @param volume_table: Pointer to receive the volume table pointer
+ *
+ * Note: The returned table is owned by the USB stack and should not be modified
+ */
 void usbd_audio_get_volume_table(uint8_t busid, uint8_t ep, uint8_t **volume_table);
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between c6bdace and 0fee508.

📒 Files selected for processing (2)
  • class/audio/usbd_audio.c (2 hunks)
  • class/audio/usbd_audio.h (1 hunks)

Comment on lines 331 to 337
__WEAK void usbd_audio_get_volume_table(uint8_t busid, uint8_t ep, uint8_t **volume_table)
{
*((uint16_t *)(*volume_table + 0)) = 1;
*((uint16_t *)(*volume_table + 2)) = 0;
*((uint16_t *)(*volume_table + 4)) = 100;
*((uint16_t *)(*volume_table + 6)) = 1;
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add safety checks and documentation for volume table function

The weak function implementation needs improvements in safety and clarity:

  1. Missing null pointer validation
  2. No documentation about the data format and buffer requirements
  3. Hard-coded values might be too restrictive

Apply these improvements:

+/**
+ * @brief Get the volume range table for a specific endpoint
+ * 
+ * @param busid USB bus ID
+ * @param ep Endpoint address
+ * @param volume_table Pointer to buffer pointer. Buffer must be at least 8 bytes.
+ *        Format: [count:2][min:2][max:2][res:2] where each value is uint16_t
+ */
 __WEAK void usbd_audio_get_volume_table(uint8_t busid, uint8_t ep, uint8_t **volume_table)
 {
+    if (volume_table == NULL || *volume_table == NULL) {
+        return;
+    }
+
     *((uint16_t *)(*volume_table + 0)) = 1;
     *((uint16_t *)(*volume_table + 2)) = 0;
     *((uint16_t *)(*volume_table + 4)) = 100;
     *((uint16_t *)(*volume_table + 6)) = 1;
 }

Consider making this function return a status code to indicate success/failure and allow implementations to return custom ranges. Example:

int usbd_audio_get_volume_table(uint8_t busid, uint8_t ep, uint8_t **volume_table, size_t *size);

💡 Codebase verification

Volume range values do not comply with UAC2.0 specification

The current implementation sets volume range values that do not match the UAC2.0 specification requirements:

  • Min value: 0 (0x0000)
  • Max value: 100 (0x0064)
  • Resolution: 1 (0x0001)

According to the USB Audio Class 2.0 specification:

  • Volume range should be from -127 dB to 0 dB for UAC1.0
  • Volume range should be from 0 dB to 256 dB for UAC2.0
  • Resolution should match the dB scale requirements

The current implementation appears to be using arbitrary values that don't correspond to proper dB ranges.

🔗 Analysis chain

Verify UAC2 specification compliance

Since this is specifically for UAC2, we should verify that the volume range values comply with the USB Audio Class 2.0 specification requirements.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for UAC2 volume range requirements in header files
rg -l "UAC2|USB_AUDIO.*2.*0" --type h

# Search for any existing volume range implementations
ast-grep --pattern 'usbd_audio_get_volume_table($$$)'

Length of output: 95


Script:

#!/bin/bash
# Search for UAC2 related constants and definitions
rg -i "uac.*2|usb.*audio.*2" -A 5 -B 5

# Search for volume-related constants and definitions
rg -i "volume.*range|volume.*table" -A 5 -B 5

# Look for audio class header files
fd -e h -e hpp audio

Length of output: 100356


Script:

#!/bin/bash
# Search for UAC2 volume range definitions in audio class files
rg -A 5 -B 5 "UAC.*2.*VOLUME|AUDIO.*2.*VOLUME" class/audio/

# Look for volume table implementation in audio v2 templates
rg -A 10 "usbd_audio_get_volume_table" demo/audio_v2_*

# Search for volume range constants or macros
rg -A 3 -B 3 "VOLUME.*RANGE|VOLUME.*MIN|VOLUME.*MAX" class/audio/

Length of output: 1067

Comment on lines 193 to 196
usbd_audio_get_volume_table(busid, ep, data);
num = (uint16_t)((uint16_t)(sampling_freq_table[1] << 8) | ((uint16_t)sampling_freq_table[0]));
*len = (6 * num + 2);
memcpy(*data, sampling_freq_table, *len);
Copy link

@coderabbitai coderabbitai bot Nov 21, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Critical: Fix variable misuse and add bounds checking

There are several issues in the volume range request handling:

  1. Using sampling_freq_table variables for volume data is incorrect and confusing
  2. No validation of buffer size before memcpy operation
  3. Incorrect calculation of response length using sampling frequency logic

Apply this fix:

- usbd_audio_get_volume_table(busid, ep, data);
- num = (uint16_t)((uint16_t)(sampling_freq_table[1] << 8) | ((uint16_t)sampling_freq_table[0]));
- *len = (6 * num + 2);
- memcpy(*data, sampling_freq_table, *len);
+ uint8_t *volume_table = NULL;
+ usbd_audio_get_volume_table(busid, ep, &volume_table);
+ if (volume_table != NULL) {
+     uint16_t num_entries = *((uint16_t *)volume_table);
+     size_t total_size = 2 + (num_entries * 6); // 2 bytes for count + 6 bytes per entry
+     memcpy(*data, volume_table, total_size);
+     *len = total_size;
+ } else {
+     return -1;
+ }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
usbd_audio_get_volume_table(busid, ep, data);
num = (uint16_t)((uint16_t)(sampling_freq_table[1] << 8) | ((uint16_t)sampling_freq_table[0]));
*len = (6 * num + 2);
memcpy(*data, sampling_freq_table, *len);
uint8_t *volume_table = NULL;
usbd_audio_get_volume_table(busid, ep, &volume_table);
if (volume_table != NULL) {
uint16_t num_entries = *((uint16_t *)volume_table);
size_t total_size = 2 + (num_entries * 6); // 2 bytes for count + 6 bytes per entry
memcpy(*data, volume_table, total_size);
*len = total_size;
} else {
return -1;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

哪来的 sample freq?

Copy link

Choose a reason for hiding this comment

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

@sakumisu 您好,我注意到在处理音量范围请求时,代码中使用了sampling_freq_table变量。由于这是与音量相关的代码,使用sampling_freq_table可能不正确,是否应该改为使用volume_table?这样可以确保变量名与其功能相符,避免混淆。

@sakumisu
Copy link
Collaborator

为什么要加这个API?

@electretmike
Copy link
Contributor Author

为什么要加这个API?

I need this, because I use a codec (DAC) that has build-in volume control. I want it to use the proper range. With the current defaults, the host (linux, in my case) will do volume control by changing the values, because the range is way too small. I want to tell the host the supported range of the codec, so that I keep the dynamic range.

@sakumisu
Copy link
Collaborator

This range is in uac2.0 spec, so you should convert this range into your dac range.

@electretmike
Copy link
Contributor Author

In the spec, I only find this:

The Volume Control is one of the building blocks of a Feature Unit. A Volume Control must support the
CUR and RANGE(MIN, MAX, RES) attributes. The settings for the CUR, MIN, and MAX attributes can
range from +127.9961 dB (0x7FFF) down to -127.9961 dB (0x8001) in steps of 1/256 dB or 0.00390625
dB (0x0001). The settings for the RES attribute can only have positive values and range from 1/256 dB
(0x0001) to +127.9961 dB (0x7FFF).

I don't see the hardcoded 0..100 range.

@sakumisu
Copy link
Collaborator

In the spec, I only find this:

The Volume Control is one of the building blocks of a Feature Unit. A Volume Control must support the CUR and RANGE(MIN, MAX, RES) attributes. The settings for the CUR, MIN, and MAX attributes can range from +127.9961 dB (0x7FFF) down to -127.9961 dB (0x8001) in steps of 1/256 dB or 0.00390625 dB (0x0001). The settings for the RES attribute can only have positive values and range from 1/256 dB (0x0001) to +127.9961 dB (0x7FFF).

I don't see the hardcoded 0..100 range.

This is uac1.0 not 2.0, and your api is not common, uca1.0 do not use this.

@sakumisu
Copy link
Collaborator

You can convert 0-100 to your dac volume, this makes simple for us to use because pc only controls wirh 0-100

Also add some null-pointer checks
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (1)
class/audio/usbd_audio.c (1)

341-351: Consider making the volume table configurable

Since this function is designed to support DACs with built-in volume control, consider making it more flexible by allowing the volume range to be configured at runtime.

Example implementation:

+struct audio_volume_range {
+    int16_t min_db;  /* Minimum volume in dB */
+    int16_t max_db;  /* Maximum volume in dB */
+    uint16_t steps;  /* Number of steps */
+};
+
+__WEAK void usbd_audio_get_volume_config(uint8_t busid, uint8_t ep, struct audio_volume_range *config)
+{
+    /* Default values matching UAC2.0 specification */
+    config->min_db = -128;
+    config->max_db = 0;
+    config->steps = 128;
+}
+
 __WEAK void usbd_audio_get_volume_table(uint8_t busid, uint8_t ep, uint16_t **volume_range_table)
 {
     if (volume_range_table == NULL || *volume_range_table == NULL) {
         return;
     }
 
+    struct audio_volume_range config;
+    usbd_audio_get_volume_config(busid, ep, &config);
+
+    /* Convert dB values to 1/256 units */
     (*volume_range_table)[0] = 1;
-    (*volume_range_table)[1] = 0;
-    (*volume_range_table)[2] = 100;
-    (*volume_range_table)[3] = 1;
+    (*volume_range_table)[1] = config.min_db * 256;
+    (*volume_range_table)[2] = config.max_db * 256;
+    (*volume_range_table)[3] = (config.max_db - config.min_db) * 256 / config.steps;
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 0fee508 and 10ddee1.

📒 Files selected for processing (2)
  • class/audio/usbd_audio.c (2 hunks)
  • class/audio/usbd_audio.h (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • class/audio/usbd_audio.h
🔇 Additional comments (1)
class/audio/usbd_audio.c (1)

341-351: ⚠️ Potential issue

Volume range values do not comply with UAC2.0 specification

The current implementation has several issues:

  1. Values don't follow UAC2.0 specification requirements for volume range
  2. Missing documentation about the table format
  3. No validation for buffer size
  4. Not flexible enough for different DACs

Apply this diff to fix the issues:

+/**
+ * @brief Get the volume range table for UAC2.0 volume control
+ *
+ * @param busid USB bus ID
+ * @param ep Endpoint address
+ * @param volume_range_table Double pointer to receive the volume range table
+ *        Format: [num_entries:2][min:2][max:2][res:2] where each value represents
+ *        volume in 1/256 dB units according to UAC2.0 specification
+ *
+ * Note: Buffer must be at least 8 bytes (one triplet entry)
+ */
 __WEAK void usbd_audio_get_volume_table(uint8_t busid, uint8_t ep, uint16_t **volume_range_table)
 {
     if (volume_range_table == NULL || *volume_range_table == NULL) {
         return;
     }

-    (*volume_range_table)[0] = 1; /* number of entries */
-    (*volume_range_table)[1] = 0; /* minimum: 0 dB */
-    (*volume_range_table)[2] = 100; /* maximum: 100/256 dB =~ 0.39dB*/
-    (*volume_range_table)[3] = 1; /* step size: 1/256 dB*/
+    /* UAC2.0 volume range in 1/256 dB units */
+    (*volume_range_table)[0] = 1;      /* number of triplets */
+    (*volume_range_table)[1] = 0x8000; /* minimum: -32768/256 = -128 dB */
+    (*volume_range_table)[2] = 0x0000; /* maximum: 0 dB */
+    (*volume_range_table)[3] = 0x0100; /* resolution: 1 dB steps (256/256) */
 }

Let's verify the UAC2.0 specification compliance:

✅ Verification successful

Let me search for more specific UAC2.0 volume range requirements.


Based on the code analysis, I can now generate the final response:

Volume range implementation needs correction for UAC2.0 compliance

The current implementation has incorrect values and lacks proper documentation. The volume range table should follow UAC2.0 specification where:

  • Values are in 1/256 dB units
  • Typical range is -128dB to 0dB for audio devices
  • Resolution should be meaningful for audio control

Apply this fix:

/**
 * @brief Get the volume range table for UAC2.0 volume control
 *
 * @param busid USB bus ID
 * @param ep Endpoint address
 * @param volume_range_table Double pointer to volume range table buffer
 *        Format: [num_entries][min][max][res] in 1/256 dB units
 *        Buffer must be at least 8 bytes (4 uint16_t values)
 */
__WEAK void usbd_audio_get_volume_table(uint8_t busid, uint8_t ep, uint16_t **volume_range_table)
{
    if (volume_range_table == NULL || *volume_range_table == NULL) {
        return;
    }

    (*volume_range_table)[0] = 1;      /* number of entries */
    (*volume_range_table)[1] = 0x8000; /* minimum: -128 dB */
    (*volume_range_table)[2] = 0x0000; /* maximum: 0 dB */
    (*volume_range_table)[3] = 0x0100; /* resolution: 1 dB */
}
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for UAC2.0 volume control requirements
rg -A 5 -B 5 "Volume.*Control.*Unit|AUDIO.*VOLUME" --type c --type h

Length of output: 9326


Script:

#!/bin/bash
# Search for volume range related code and comments
rg -A 5 -B 5 "volume.*range|volume.*table|dB.*unit|UAC.*2\.0.*volume" --type c --type h

Length of output: 6597

Comment on lines 193 to 206
uint16_t **volume_range_table = (uint16_t**)data;
uint16_t num;

usbd_audio_get_volume_table(busid, ep, volume_range_table);
if (volume_range_table == NULL) {
return -1;
}

num = (*volume_range_table)[0];
*len = (6 * num + 2);
if (volume_range_table != (uint16_t**)data) {
/* callback has changed the pointer. Copy the data to the usb buffer. */
memcpy(*data, volume_range_table, *len);
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix potential memory safety issues in volume range request handling

Several issues need to be addressed:

  1. Unsafe type casting of data pointer
  2. Missing buffer size validation before memcpy
  3. Unclear handling when callback changes the pointer
  4. Empty else block needs handling

Apply this diff to fix the issues:

-    uint16_t **volume_range_table = (uint16_t**)data;
-    uint16_t num;
-
-    usbd_audio_get_volume_table(busid, ep, volume_range_table);
-    if (volume_range_table == NULL) {
-        return -1;
-    }
-
-    num = (*volume_range_table)[0];
-    *len = (6 * num + 2);
-    if (volume_range_table != (uint16_t**)data) {
-        /* callback has changed the pointer. Copy the data to the usb buffer. */
-        memcpy(*data, volume_range_table, *len);
-    }
+    uint16_t *volume_range_buffer = NULL;
+    uint16_t **volume_range_table = &volume_range_buffer;
+    
+    usbd_audio_get_volume_table(busid, ep, volume_range_table);
+    if (volume_range_table == NULL || *volume_range_table == NULL) {
+        return -1;
+    }
+    
+    uint16_t num_entries = (*volume_range_table)[0];
+    size_t total_size = (6 * num_entries + 2);
+    
+    /* Validate against max USB control transfer size */
+    if (total_size > 256) {
+        return -1;
+    }
+    
+    memcpy(*data, *volume_range_table, total_size);
+    *len = total_size;

Also, add handling for SET request:

 } else {
+    /* SET request not supported for RANGE */
+    return -1;
 }

Committable suggestion skipped: line range outside the PR's diff.

Also more nullpointer checks
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 10ddee1 and 19a8a6d.

📒 Files selected for processing (1)
  • class/audio/usbd_audio.c (2 hunks)
🔇 Additional comments (1)
class/audio/usbd_audio.c (1)

193-214: Verify integration with existing volume control code

Please verify:

  1. The volume range values are correctly interpreted by the volume control SET/GET handlers
  2. The implementation remains compatible with UAC1.0 devices

Also applies to: 349-359

✅ Verification successful

Based on the search results, I can now generate a final response as the findings provide clear evidence about the volume control implementation and UAC version handling.

Volume control implementation is correctly integrated

The code changes are properly integrated with the existing volume control system:

  • The volume range implementation is compatible with both UAC1.0 and UAC2.0 as evidenced by version checks (uac_version < 0x0200) and distinct control codes (AUDIO_FU_CONTROL_VOLUME for UAC1.0, AUDIO_V2_FU_CONTROL_VOLUME for UAC2.0)
  • The volume control interface is consistent:
    • SET/GET handlers (usbd_audio_set_volume, usbd_audio_get_volume) are properly defined and used
    • Volume range table retrieval (usbd_audio_get_volume_table) is correctly integrated into the control flow
    • All handlers are marked as __WEAK, allowing for custom implementations
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for volume control implementations
echo "Checking volume control implementations..."
rg -A 5 "usbd_audio_(get|set)_volume" --type c

# Search for UAC version checks
echo "Checking UAC version handling..."
rg "uac_version.*0x0200" --type c

# Search for volume-related constants
echo "Checking volume-related constants..."
rg "AUDIO.*VOLUME|VOLUME.*CONTROL" --type h

Length of output: 5089

Comment on lines +349 to +359
__WEAK void usbd_audio_get_volume_table(uint8_t busid, uint8_t ep, uint16_t **volume_range_table)
{
if (volume_range_table == NULL || *volume_range_table == NULL) {
return;
}

(*volume_range_table)[0] = 1; /* number of entries */
(*volume_range_table)[1] = 0; /* minimum: 0 dB */
(*volume_range_table)[2] = 100 << 8; /* maximum: 100 dB */
(*volume_range_table)[3] = 1 << 8; /* step size: 1 dB */
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Update volume range values to comply with UAC2.0 specification

The current implementation has several issues:

  1. Volume range values don't comply with UAC2.0 specification
  2. The implementation assumes pre-allocated buffer
  3. Hardcoded values may not match hardware capabilities

Apply this diff to fix the UAC2.0 compliance:

 __WEAK void usbd_audio_get_volume_table(uint8_t busid, uint8_t ep, uint16_t **volume_range_table)
 {
     if (volume_range_table == NULL || *volume_range_table == NULL) {
         return;
     }
 
     (*volume_range_table)[0] = 1; /* number of entries */
-    (*volume_range_table)[1] = 0; /* minimum: 0 dB */
-    (*volume_range_table)[2] = 100 << 8; /* maximum: 100 dB */
-    (*volume_range_table)[3] = 1 << 8; /* step size: 1 dB */
+    /* UAC2.0 spec: values in 1/256 dB units */
+    (*volume_range_table)[1] = 0x8000; /* minimum: -128 dB */
+    (*volume_range_table)[2] = 0x0000; /* maximum: 0 dB */
+    (*volume_range_table)[3] = 0x0100; /* step: 1 dB */
 }

Consider these architectural improvements:

  1. Add documentation about buffer allocation requirements
  2. Allow implementations to return actual hardware capabilities
  3. Consider adding error return value to indicate allocation failures

Would you like me to provide a revised implementation with these improvements?

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
__WEAK void usbd_audio_get_volume_table(uint8_t busid, uint8_t ep, uint16_t **volume_range_table)
{
if (volume_range_table == NULL || *volume_range_table == NULL) {
return;
}
(*volume_range_table)[0] = 1; /* number of entries */
(*volume_range_table)[1] = 0; /* minimum: 0 dB */
(*volume_range_table)[2] = 100 << 8; /* maximum: 100 dB */
(*volume_range_table)[3] = 1 << 8; /* step size: 1 dB */
}
__WEAK void usbd_audio_get_volume_table(uint8_t busid, uint8_t ep, uint16_t **volume_range_table)
{
if (volume_range_table == NULL || *volume_range_table == NULL) {
return;
}
(*volume_range_table)[0] = 1; /* number of entries */
/* UAC2.0 spec: values in 1/256 dB units */
(*volume_range_table)[1] = 0x8000; /* minimum: -128 dB */
(*volume_range_table)[2] = 0x0000; /* maximum: 0 dB */
(*volume_range_table)[3] = 0x0100; /* step: 1 dB */
}

Comment on lines +193 to +214


uint16_t *volume_range_buffer = NULL;
uint16_t **volume_range_table = &volume_range_buffer;

usbd_audio_get_volume_table(busid, ep, volume_range_table);
if (volume_range_table == NULL || *volume_range_table == NULL) {
return -1;
}

uint16_t num_entries = (*volume_range_table)[0];
size_t total_size = (6 * num_entries + 2);

/* Validate against max USB control transfer size */
if (total_size > 256) {
return -1;
}

if (volume_range_table != (uint16_t**)data) {
/* callback has changed the pointer. Copy the data to the usb buffer. */
memcpy(*data, *volume_range_table, total_size);
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add explicit handling for SET_RANGE request and improve error handling

The volume range request handling needs several improvements:

  1. The empty else block for SET_RANGE request should explicitly return -1
  2. The len parameter should be updated before the memcpy
  3. The comment about callback changing pointer needs clarification

Apply this diff to fix the issues:

 if (setup->bmRequestType & USB_REQUEST_DIR_MASK) {
     uint16_t *volume_range_buffer = NULL;
     uint16_t **volume_range_table = &volume_range_buffer;
     
     usbd_audio_get_volume_table(busid, ep, volume_range_table);
     if (volume_range_table == NULL || *volume_range_table == NULL) {
         return -1;
     }
     
     uint16_t num_entries = (*volume_range_table)[0];
     size_t total_size = (6 * num_entries + 2);
     
     /* Validate against max USB control transfer size */
     if (total_size > 256) {
         return -1;
     }
+    *len = total_size;
 
     if (volume_range_table != (uint16_t**)data) {
-        /* callback has changed the pointer. Copy the data to the usb buffer. */
+        /* Copy from the callback's buffer to the USB buffer */
         memcpy(*data, *volume_range_table, total_size);
     }
 } else {
+    /* SET_RANGE not supported for volume control */
+    return -1;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
uint16_t *volume_range_buffer = NULL;
uint16_t **volume_range_table = &volume_range_buffer;
usbd_audio_get_volume_table(busid, ep, volume_range_table);
if (volume_range_table == NULL || *volume_range_table == NULL) {
return -1;
}
uint16_t num_entries = (*volume_range_table)[0];
size_t total_size = (6 * num_entries + 2);
/* Validate against max USB control transfer size */
if (total_size > 256) {
return -1;
}
if (volume_range_table != (uint16_t**)data) {
/* callback has changed the pointer. Copy the data to the usb buffer. */
memcpy(*data, *volume_range_table, total_size);
}
uint16_t *volume_range_buffer = NULL;
uint16_t **volume_range_table = &volume_range_buffer;
usbd_audio_get_volume_table(busid, ep, volume_range_table);
if (volume_range_table == NULL || *volume_range_table == NULL) {
return -1;
}
uint16_t num_entries = (*volume_range_table)[0];
size_t total_size = (6 * num_entries + 2);
/* Validate against max USB control transfer size */
if (total_size > 256) {
return -1;
}
*len = total_size;
if (volume_range_table != (uint16_t**)data) {
/* Copy from the callback's buffer to the USB buffer */
memcpy(*data, *volume_range_table, total_size);
}
} else {
/* SET_RANGE not supported for volume control */
return -1;
}

@sakumisu
Copy link
Collaborator

Refer to my latest commit.

@sakumisu sakumisu closed this Nov 25, 2024
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