The mysterious __packed uint32_t in USB_ReadPacket and USB_WritePacket

[PHolt.1]

Does anyone here know what is the point of this construct, which GCC (shipped with Cube IDE) doesn't like anyway?

I posted details here

https://www.eevblog.com/forum/programming/packed-attribute-warning/

[Pavel A.]

This is for read/write a multibyte value (32-bit in this case) from/to unaligned location.

For gcc, __packed should be defined somewhere as __attribute__((packed)).

Alternatively, for any compiler use the CMSIS (ARM-defined) macros __UNALIGNED_UINT32_READ, __UNALIGNED_UINT32_WRITE - see here.

[PHolt.1]

Can you explain what this does?

As I wrote, the compiler says the __packed is not supported.

The 32F417 supports unaligned uint32s so it should work anyway, just a tiny bit slower, and how could __packed avoid that slowdown?

[waclawek.jan]

You probably have a rather old version of Cube, as even in the oldest version of blahblah_ll_usb.c I can find in the official github CubeF4 repository (2009) these reads/writes are guarded by __UNALIGNED_UINT32_WRITE/__UNALIGNED_UINT32_READ which only in the case of using armcc expand into __packed.

This in case of 'F4 does nothing (maybe supresses compiler warning in some very particular cases); it was most probably installed as Cube USB driver had fallen over exactly this issue (unaligned reads/writes) in the Cortex-M0/M0+-based mcus.

JW

[PHolt.1]

I am on the latest Cube (v10.1) but the source code came out of Cube v1.4 or so (as detailed in the EEVBLOG thread) maybe 3 years ago and any links to Cube-supplied sources were broken after that (for obvious and very good reasons: a massive amount of regression testing would be needed every time Cube is updated). But certainly well after 2009; this project was only started in 2016.

It is possible the USB code came from elsewhere, but it clearly is ST code...

OK, so it looks like this is fossil code (for the 32F4; other 32F CPUs seem to need alignment) but would I be right in that if the buffers (64 byte, generally, for USB FS) were to be unaligned, it should still work, although a tiny bit slower?

[waclawek.jan]

> I am on the latest Cube (v10.1) but the source code came out of Cube v1.4

Cube is originally the generic name for the "library" (sometimes called, very, very, very incorrectly as "firmware") - here, CubeF4. What you are referring to is probably version of CubeIDE.

> any links to Cube-supplied sources were broken after that (for obvious and very good reasons

If those reasons are obvious, then why do you keep updating CubeIDE with all the associated toolchains?

Nonetheless, this particular warning was probably present in the original combination, too.

I don't subscribe to the notion of wholesale updating either, but I don't consider the "frozen" version of "libraries" (not Cube, I don't use Cube) to be sacred and change them either by comparing to new versions, or according to my own findings, as I see fit.

> It is possible the USB code came from elsewhere, but it clearly is ST code...

I never said it's not ST code. What I've said is, that they fixed a real and palpable issue in the 'F0/'L0 ('G0 was not around at that time, IIRC) variant of Cube/USB, and this effort left its marks on the non-Cortex-M0/M0+ variants too.

> would I be right in that if the buffers (64 byte, generally, for USB FS) were to be unaligned, it should still work, although a tiny bit slower?

In Cortex-M3/M4/M7-based STM32, those particular lines, yes.

JW

[PHolt.1]

"why do you keep updating CubeIDE with all the associated toolchains?"

1) In the hope of bugs getting fixed. Most of the obvious ones aren't (like the random file opening) but there are a few little fixes here and there.

2) Because eventually others will be writing modules for this product and I can't keep telling them to use the same version of Cube (which ST remove from their server after a few months anyway, so I would have to issue it, which in theory is contrary to the ST redist Ts and Cs). Yes, I know, this could be a problem eventually with stuff breaking, so I will have to freeze the Cube version one day. But not yet, while I can keep testing it (and hoping for more bug fixes from ST).

"change them either by comparing to new versions, or according to my own findings, as I see fit."

If doing that, one needs to do a line by line comparison (there are tools) and one needs to understand the impact of the changes (which is often hard).

"this effort left its marks on the non-Cortex-M0/M0+ variants too."

"In Cortex-M3/M4/M7-based STM32, those particular lines, yes."

OK; thank you... that answers my original Q.

[waclawek.jan]

> If doing that, one needs to do a line by line comparison (there are tools) and one needs to understand the impact of the changes (which is often hard).

Yes. Programming is hard. Using others' work often makes it not easier in the long run. Did I mention I don't use Cube?

JW

[PHolt.1]

Cube is just an IDE. It is an editor (a good one, except for not supporting simple bookmarks) and an auto makefile generator. It works well, generally.

The "Cube code" is from Cube MX which is the ST "code generator". The guy who set up this project a few years ago used Cube MX, and some online sources I think, to put together the original skeleton where one had the ST 407G-DISC1 board running with ETH, USB CDC, USB MSC, LWIP, FreeRTOS. That code, mostly not updated since, seems to work well and I see no reason to update it other than perhaps as a way to look for where bugs may lie. My board is based on the DISC1 circuit, with lots of mods, but it all worked first time.

Re using others' work, I am suspicious of anything on github (which contains a vast % of garbage and "work in progress") but I expect ST code to work - even if not optimal due to the "code generator" not being granular enough, like not using DMA on USB FS but testing for dma in all their USB FS functions.

ST not participating here doesn't help them, although if they are even half smart they will do it covertly.

[JimYuill]

UPDATE 1/3/22: I had misquoted the gcc docs. That's now fixed (changed text is bold).

I've run into the same problem, in stm32f7xx_ll_usb.c.

I don't have answers, but some additional clues about the problem.

I don't know where that particular file came from, that I have, but the header states:

"@author MCD Application Team:"

Copyright (c) 2017 STMicroelectronics.

Based on some info by the IDE-project's creator, I suspect the IDE-project was originally created by Atolic True Studio, then converted to an STM32CubeIDE project, probably in 2019.

Lines that get warning messages are:

USBx_DFIFO((uint32_t)ch_ep_num) = *((__packed uint32_t *)pSrc);

*(__packed uint32_t *)pDest = USBx_DFIFO(0U);

The warning message is:

.../STM32F7xx_HAL_Driver/Src/stm32f7xx_ll_usb.c:958:7: warning: 'packed' attribute ignored for type 'uint32_t *' {aka 'long unsigned int *'} [-Wattributes]

__packed is a macro that expands to: __attribute__((__packed__))

The use of packed on a variable is puzzling.

The gcc docs for packed differ regarding whether packed can be used with a variable.

The gcc 4.4.7 docs state packed can be applied to a variable:

5.33 Specifying Attributes of Variables

packed

The packed attribute specifies that a variable or structure field should have the smallest possible alignment—one byte for a variable, and one bit for a field, unless you specify a larger value with the aligned attribute.

  

gcc's current docs (v11?) [UPDATE:] appear to say the only variable packed can be applied to is a "structure member", e.g., not a variable outside of a structure.

6.34.1 Common Variable Attributes

packed

The packed attribute specifies that a structure member should have the smallest possible alignment—one bit for a bit-field and one byte otherwise, unless a larger value is specified with the aligned attribute. The attribute does not apply to non-member objects.

I tried using __packed on a variable, with gcc 4.4, and I got the same error message specified above.

#define __packed __attribute__((packed))
unsigned int y __packed;

I checked the file stm32f7xx_ll_usb.c, that was generated by a newer version of STM32CubeIDE.

The two lines with __packed no longer use __packed.

They appear to be replaced by, these two lines, respectively:

USBx_DFIFO((uint32_t)ch_ep_num) = __UNALIGNED_UINT32_READ(pSrc);

  __UNALIGNED_UINT32_WRITE(pDest, USBx_DFIFO(0U));

Also, the way the pointers are incremented is changed (the pointers pSrc and PDest).

So, the use of __packed was replaced by another mechanism to support unaligned reads and write.

This implies the __packed was needed to serve that purpose.

And, simply removing __packed, or ignoring the warning, might not be benign.

I'm working with a bunch of IDE-projects that issue this warning message, and the projects still seem to work ok. Though, it's proof-of-concept code, and not production code.

Again, I'm just posting additional clues about the problem, for anyone else investigating it.

[Pavel A.]

> __packed is a macro that expands to: __attribute__((__packed__))

​

IMHO this can be explained as follows (not an authoritative opinion of course, please feel free to correct):

The __packed in that source is a keyword of IAR compiler. Later when support for gcc had been added, __packed was defined as "__attribute__((__packed__))".

But for gcc, meaning of __attribute__((__packed__)) has a subtle difference, in some contexts where the IAR __packed is valid, the gcc attribute is not.

Basically gcc has two distinct contexts for __attribute__((packed)): on variables and on structs (see here)

Modern GCC compiler warns if the attribute appears in awkward place where it has no effect,

but where IAR compiler may accept it.

​

Similar thing about under-cover implementation of __UNALIGNED... (which are ARM-specific CMSIS definitions). Different compilers have subtle differences.

Bottom line:

• Do not ignore compiler warnings. They are your friends.
• Try to understand the warnings and read the fine manuals, as time allows.
• Look at the generated code (disassembly)

​

[JimYuill]

PavelA., great info, thanks.

The post you linked-to mentions that the gcc docs for "packed" have different sections for "variable attributes" and "type attributes".

I realized that in my post, I misquoted the gcc docs. In quoting the current gcc docs, I accidentally quoted the "type attributes" specs instead of the "variable attribute" specs.

I updated my post to fix that.

The fix was instructive, as those specs specifically say that for variables, "packed" can only be specified for "structure members".

[PHolt.1]

I solved it by removing the "packed" and ensure that the buffer is aligned (by the fact that it comes from a malloc() which has a guaranteed 8- alignment).

Of course as per good embedded practice there is no free(); the malloc() is done at startup only.

Also on the 32F4 you can do word ops without alignment being needed; other types differ I believe. It is slower though if not aligned, although I doubt anybody will tell the difference in practice.