Add IAR warning flags to CMake build and resolve warnings#3528
Conversation
There was a problem hiding this comment.
Pull request overview
Adds IAR-specific warning flags to the CMake build and adjusts code patterns to eliminate IAR warning diagnostics (notably around volatile access ordering and unreachable code).
Changes:
- Add IAR warning/error flags in the common CMake family configuration.
- Refactor FIFO helpers to avoid IAR volatile access ordering warnings without compiler pragmas.
- Remove unreachable returns / tidy control-flow and casts to resolve compiler warnings across examples and DFU.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/osal/osal_freertos.h | Uses compile-time branching for ISR spinlock behavior to avoid warnings. |
| src/common/tusb_fifo.h | Reworks volatile reads in inline FIFO helpers; minor comment cleanup. |
| src/common/tusb_fifo.c | Reworks volatile reads passed as function args to avoid IAR warnings. |
| src/class/dfu/dfu_device.c | Adds explicit casts to satisfy enum/type warnings. |
| hw/bsp/family_support.cmake | Introduces IAR warning flags and wires them into target compile options. |
| examples/host/msc_file_explorer/src/msc_app.c | Removes unreachable return. |
| examples/host/msc_file_explorer/src/main.c | Removes unreachable return. |
| examples/host/midi_rx/src/main.c | Removes unreachable return. |
| examples/host/device_info/src/main.c | Removes unreachable return. |
| examples/host/bare_api/src/main.c | Removes unreachable return. |
| examples/dual/host_hid_to_device_cdc/src/main.c | Removes unreachable return. |
| examples/device/uac2_speaker_fb/src/main.c | Adds braces; refactors expression to reduce warning/noise. |
| examples/device/net_lwip_webserver/src/main.c | Removes unreachable return. |
| examples/device/msc_dual_lun/src/main.c | Removes unreachable return. |
| examples/device/hid_boot_interface/src/main.c | Removes unreachable return. |
| examples/device/dynamic_configuration/src/msc_disk.c | Simplifies unsupported SCSI handling; adds explicit unused markers. |
| examples/device/cdc_uac2/src/main.c | Removes unreachable return. |
| examples/device/cdc_msc_freertos/src/msc_disk.c | Simplifies unsupported SCSI handling; adds explicit unused markers. |
| examples/device/cdc_msc/src/msc_disk.c | Fixes grammar in comment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| set(WARN_FLAGS_IAR | ||
| --warnings_are_errors | ||
| --diag_suppress=Pa089 | ||
| --diag_suppress=Pe236 | ||
| ) | ||
|
|
There was a problem hiding this comment.
Applying --warnings_are_errors plus globally suppressing specific diagnostics at the family level can make downstream/3rd-party code builds fail unexpectedly while also hiding those suppressed warning classes across the entire target. Consider making --warnings_are_errors opt-in (e.g., via a CMake option) and scoping --diag_suppress to only the specific targets/files that need it, with a short comment explaining what Pa089/Pe236 represent and why suppression is safe.
| set(WARN_FLAGS_IAR | |
| --warnings_are_errors | |
| --diag_suppress=Pa089 | |
| --diag_suppress=Pe236 | |
| ) | |
| # Control how strictly IAR treats warnings and whether specific diagnostics | |
| # are suppressed. These options are provided to avoid forcing downstream or | |
| # 3rd-party code to build with warnings-as-errors or hidden diagnostics. | |
| option(IAR_WARNINGS_ARE_ERRORS "Treat all IAR compiler warnings as errors" OFF) | |
| option(IAR_SUPPRESS_DIAGNOSTICS "Suppress specific IAR diagnostics (Pa089, Pe236)" ON) | |
| set(WARN_FLAGS_IAR) | |
| if (IAR_WARNINGS_ARE_ERRORS) | |
| list(APPEND WARN_FLAGS_IAR | |
| --warnings_are_errors | |
| ) | |
| endif() | |
| if (IAR_SUPPRESS_DIAGNOSTICS) | |
| # Pa089: disabled optimization messages that are noisy but not correctness issues. | |
| # Pe236: controlling expression is constant; common in embedded register/bitfield code. | |
| # These are considered safe to suppress for this BSP, but can be disabled via | |
| # -DIAR_SUPPRESS_DIAGNOSTICS=OFF if a project prefers to see them. | |
| list(APPEND WARN_FLAGS_IAR | |
| --diag_suppress=Pa089 | |
| --diag_suppress=Pe236 | |
| ) | |
| endif() |
| const uint16_t wr_idx = f->wr_idx; | ||
| const uint16_t rd_idx = f->rd_idx; | ||
| return tu_min16(tu_ff_overflow_count(f->depth, wr_idx, rd_idx), f->depth); |
There was a problem hiding this comment.
The code now intentionally snapshots volatile indices into locals to avoid IAR's Pa082 (unspecified ordering of volatile accesses). Since the previous pragma-based explanation was removed, add a brief comment near these snapshots (or once above the related inline helpers) explaining that this pattern is required to enforce a defined volatile read order for IAR and similar compilers.
| // - READ10 and WRITE10 has their own callbacks | ||
| int32_t tud_msc_scsi_cb (uint8_t lun, uint8_t const scsi_cmd[16], void* buffer, uint16_t bufsize) { | ||
| // read10 & write10 has their own callback and MUST not be handled here | ||
| (void) lun; |
There was a problem hiding this comment.
lun is explicitly marked unused but is still used in the tud_msc_set_sense(lun, ...) call. This is harmless but misleading for readers and can mask real unused-parameter situations during refactors. Drop the (void) lun; line (keep (void) scsi_cmd; if you intend to ignore the command), or alternatively keep using scsi_cmd[0] in a minimal switch to justify the parameter.
| (void) lun; |
| // Set Sense = Invalid Command Operation | ||
| (void) tud_msc_set_sense(lun, SCSI_SENSE_ILLEGAL_REQUEST, 0x20, 0x00); |
There was a problem hiding this comment.
lun is explicitly marked unused but is still used in the tud_msc_set_sense(lun, ...) call. This is harmless but misleading for readers and can mask real unused-parameter situations during refactors. Drop the (void) lun; line (keep (void) scsi_cmd; if you intend to ignore the command), or alternatively keep using scsi_cmd[0] in a minimal switch to justify the parameter.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| (void) tud_msc_set_sense(lun, SCSI_SENSE_ILLEGAL_REQUEST, 0x20, 0x00); | ||
|
|
||
| return -1; | ||
| return -1; // stall/failed command request; |
Check notice
Code scanning / CodeQL
Commented-out code Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the issue without changing functionality, we just need to adjust the comment so it no longer looks code-like to the analyzer while still documenting the meaning of return -1;. The best approach is to rewrite it as a full sentence in natural language, possibly expanding abbreviations, so it’s clearly not intended as code. We only need to edit the comment on line 337 in examples/device/cdc_msc_freertos/src/msc_disk.c.
Concretely, replace // stall/failed command request; with something like // Indicate a stalled or failed command request.. This keeps the same intent but removes the trailing semicolon and uses normal prose, which should satisfy the static analysis rule. No additional methods, imports, or definitions are needed.
| @@ -334,7 +334,7 @@ | ||
| // Set Sense = Invalid Command Operation | ||
| (void) tud_msc_set_sense(lun, SCSI_SENSE_ILLEGAL_REQUEST, 0x20, 0x00); | ||
|
|
||
| return -1; // stall/failed command request; | ||
| return -1; // Indicate a stalled or failed command request. | ||
| } | ||
|
|
||
| #endif |
| (void) tud_msc_set_sense(lun, SCSI_SENSE_ILLEGAL_REQUEST, 0x20, 0x00); | ||
|
|
||
| return -1; | ||
| return -1; // stall/failed command request; |
Check notice
Code scanning / CodeQL
Commented-out code Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
In general, to address “commented-out code” findings, either delete truly dead commented code or reword comments so they clearly read as prose instead of code. Here, the best fix is to keep the documentation but rewrite the comment so it no longer resembles a code statement (for example, remove the trailing semicolon and phrase it as an explanatory sentence). This maintains existing functionality and behavior exactly while satisfying the static analysis rule.
Concretely, in examples/device/dynamic_configuration/src/msc_disk.c, change the comment on line 228 that currently reads // stall/failed command request; to a clearer explanation such as // Return value -1 indicates a stalled or failed command request.. No additional methods, imports, or definitions are required; we are only editing this comment.
| @@ -225,7 +225,7 @@ | ||
| // Set Sense = Invalid Command Operation | ||
| (void) tud_msc_set_sense(lun, SCSI_SENSE_ILLEGAL_REQUEST, 0x20, 0x00); | ||
|
|
||
| return -1; // stall/failed command request; | ||
| return -1; // Return value -1 indicates a stalled or failed command request. | ||
| } | ||
|
|
||
| #endif |
Size Difference ReportBecause TinyUSB code size varies by port and configuration, the metrics below represent the averaged totals across all example builds. Note: If there is no change, only one value is shown. Changes >1% in sizeNo entries. Changes <1% in size
No changes
|
hathach
force-pushed
the
improve-iar-warnings
branch
from
0e0e920 to
94baf39
Compare
### Description This pull request adds IAR-specific warning flags to the CMake build system and resolves associated warnings