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

do not apply storage attributes to member of struct #1698

Merged
merged 2 commits into from
Oct 25, 2022

Conversation

battlesnake
Copy link
Contributor

@battlesnake battlesnake commented Oct 20, 2022

Describe the PR
Compilation fails when audio interrupt-endpoint is enabled, due to storage attributes on a struct member.
I removed the attributes.

Additional context

Copy link
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

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

it does not fail with my compiling. This is required for some mcu. Could you provide info on your setup

@battlesnake
Copy link
Contributor Author

battlesnake commented Oct 21, 2022

This is using GCC 12.2.0, st/synopsys port, STM32H7.

The attributes (CFG_TUSB_MEM_SECTION CFG_TUSB_MEM_ALIGN) are being applied to a member of a struct:

struct {
  ...
  CFG_TUSB_MEM_SECTION CFG_TUSB_MEM_ALIGN uint8_t ep_int_ctr_buf[CFG_TUD_AUDIO_INT_CTR_EP_IN_SW_BUFFER_SIZE];
  ...
}

I don't think it makes sense (regarding memory layout) for some members of a struct to be in a different linker section to the rest of the struct? GCC certainly doesn't like this.

The only instance of the struct is (around line 413):

CFG_TUSB_MEM_SECTION audiod_function_t _audiod_fct[CFG_TUD_AUDIO];

So the struct (and therefore its members) are already in the section denoted by CFG_TUSB_MEM_SECTION.
The alignment is a separate issue, perhaps that attribute should stay.

Setting CFG_TUD_AUDIO_INT_CTR_EPSIZE_IN to non-zero (e.g. 6) enables that struct member.
If you have CFG_TUSB_MEM_SECTION defined with a specific section, then this combination should be sufficient to trigger a compiler error.

For configuration, I have:

#define _d1ram_bss __attribute__((__section__(".ram_d1.bss")))
...
#define dma_align_bytes (32)
#define _dma_align __attribute__((__aligned__(dma_align_bytes)))
...
#define CFG_TUSB_MEM_SECTION _d1ram_bss
#define CFG_TUSB_MEM_ALIGN _dma_align

#define CFG_TUD_AUDIO_INT_CTR_EPSIZE_IN 6

Which, expanded, is:

#define CFG_TUSB_MEM_SECTION __attribute__((__section__(".ram_d1.bss")))
#define CFG_TUSB_MEM_ALIGN __attribute__((__aligned__(32)))

#define CFG_TUD_AUDIO_INT_CTR_EPSIZE_IN 6

@battlesnake
Copy link
Contributor Author

battlesnake commented Oct 22, 2022

Currently, the ep_int_ctr member is used, but never assigned.

So I'm guessing that I'm the first person to try building a UAC2 configuration with interrupt/status endpoint enabled, and the feature currently isn't fully implemented in tinyusb yet.

I'm trying to implement it, but not sure if I'll succeed.

@hathach
Copy link
Owner

hathach commented Oct 24, 2022

I don't think it makes sense (regarding memory layout) for some members of a struct to be in a different linker section to the rest of the struct? GCC certainly doesn't like this.

Oh, this is right, please remove the CFG_TUSB_MEM_SECTION but keep the CFG_TUSB_MEM_ALIGN

@battlesnake
Copy link
Contributor Author

Done

Copy link
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

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

perfect, thank you

@hathach hathach merged commit 28f49c0 into hathach:master Oct 25, 2022
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