Packed attribute warning - what is a "packed uint32_t"?

[peter-h]

Get off your high horse

I guess this is cultural but there is no need to be rude.

Just had a look. 50MB. Yeah, I admire you for knowing it all. Especially as you got paid for reading it I've never had a job since leaving univ (44 years ago) so never got paid for learning something.

[ataradov]

Especially as you got paid for reading it

The other way around, I've got the job because I read stuff. I do it for fun in my spare time.

[newbrain]

The other way around, I've got the job because I read stuff. I do it for fun in my spare time.

QFT.
Some years ago I changed job - internally, we are big - and one of the reasons I got the new job was that the person checking my technical competence did not expect me to know stuff totally unrelated to my previous positions.
Recently, I was surprised to get an offer from an external firm that provided us a course - same reason, as reported by the instructor (not interested, though).

I do not consider myself especially smart, and I'm a bit lazy, but expanding one's own knowledge pays off, where my innate charm doesn't cut it.

[peter-h]

I recommend a google on the early days of USB, the criticism of the messy spec, widespread compatibility issues, and then how it developed, and who drove the standardisation (Intel, with their motherboard chip monopoly) which eventually produced a workable solution.

You just need to be a bit "old" and probably a lot of it is before the internet got going.

There is still a lot of funny stuff deep in there e.g. how writes to slow FLASH are handled. I did some long threads on it when looking for a solution for my FatFS impementation in a 15ms sector-write Adesto FLASH. I got slagged off for doing this 15ms write in the USB ISR (the interrupt which occurs when the 8th 64 byte MSC packet has arrived, and the 512 byte sector is ready to be written) but nobody had the slightest clue about how to do it "puritanically". One purist told me I was supposed to write a "driver" for the FLASH (which solves precisely nothing). Basically, it appears, returning any of the BUSY status codes breaks the Windows driver. The stuff about it retrying on a BUSY status appears to be BS. I spent days or weeks of my life going down that rabbit hole. Then it turned out that FLASH sticks do exactly what I was doing: they hang in the ISR for each sector write and return OK when it is finished. This approach works on all windows versions from XP onwards, and on Linux. The timeout on Windows MSC is huge - probably a second or so because I can single step through USB code in the debugger and it continues to run.

It's easy to criticise...

[ataradov]

I'm well aware of USB history. It is virtually impossible to design a perfect standard out of the box. And they did pretty well and then worked to solve issues that came up. It is easy to criticize indeed, especially when you have not done anything on a similar scale and adoption.

Your USB struggles are related to you not understanding the standard and ST USB stack. They have nothing to do with the actual standard for USB or MSC.

[coppice]

I'm well aware of USB history. It is virtually impossible to design a perfect standard out of the box. And they did pretty well and then worked to solve issues that came up. It is easy to criticize indeed, especially when you have not done anything on a similar scale and adoption.

Pretty well? Try telling that to all the peripheral makers who pumped money into multiple generations of USB devices that didn't sell because the whole chain wasn't ready, and had become outdated by the time it was. Senior people at Intel quipped in retrospect that they took less time to fight the might of Germany and Japan than it took to get USB out the door fully formed and usable.

[ataradov]

Try telling that to all the peripheral makers who pumped money into multiple generations of USB devices that didn't sell because the whole chain wasn't ready,

That will always happen with new standards. Also, multiple generations? USB 1.0 had issues, but it was released for 2 years before USB 1.1 addresses most of them and was perfectly usable. And by that time native OS support was common.

In the late 90s manufacturers released a lot of junk that was destined to fail, not USB fault.

USB is collaborative project, it would take longer time to release if you compare to proprietary stuff.

I'm not saying that USB is perfect, knowing all the issues, I would not design a new interface to be like that. But back then they did not know and had no good references.

[SiliconWizard]

Given how fragmented interfaces were before USB in general, and plug-and-play being a relatively brand-new feature overall at this time, it was still a great achievement.

And now that it's been around for about 25 years, with backwards compatibility always maintained. Uh yeah. Calling it a failure?

For people stating this kind of absurdity, please let us know what *you* have achieved. We can certainly all learn from that!

[peter-h]

Your USB struggles are related to you not understanding the standard and ST USB stack

I posted about it here in the hope that a true USB expert could set me on the correct course. It would appear that you missed the opportunity

And now that it's been around for about 25 years

Not really surprising that it works, after years Bluetooth might get there one day, too. Too late for all the people whose 5 year old car system has stopped working after a phone update... I will never try to develop a BT product; it would be a nightmare for any manufacturer who has to support their products (which doesn't apply to 99% of participants in the retail space).

please let us know what *you* have achieved.

I have achieved nothing, which is why I am here: to learn from the masters
https://en.wikipedia.org/wiki/Standing_on_the_shoulders_of_giants

[wek]

So, wrapping it all up,

- __packed attribute is already not there in said functions the current/recent incarnations of Cube
- it's a Keil/ARM compiler native qualifier, the attribute into which it converts for gcc does nothing (i.e. is both ineffective and harmless) except it throws a warning
- said function moves words into USB hardware (FIFO) from buffer in RAM passed through pointer
- pointer to buffer is best to be word-aligned for speed, but there are legitimate user cases where users want to use unaligned pointer (e.g. zero-copy higher-level protocols)
- plain dereferencing unaligned pointer to word throws error in 'CM0/'CM0+ and works somewhat less efficiently in 'CM3/'CM4/'CM7
- facing this error, Cube authors decided to allow for unaligned pointer for users convenience at the cost of more work for them
- for Keil/ARM compiler it's easy, just adding __packed moves the problem to the compiler which does whatever is needed (i.e. generates multiple split accesses)
- I wouldn't be surprised to learn that IAR has similar feature
- this may be not optimal as there are algorithms through which most accesses still can be word-wise at the cost of dis/reassembling words in software, but here we are
- Cube authors initially probably did not realize that this does not work in the same way in gcc, IIRC in current Cube there is explicit disassembly/reassembly as needed for gcc
- in the 'CM3/4/7 version, __packed is there too, maybe to suppress warning about unaligned access? but as said above, is mostly harmless
- in the current 'CM3/4/7 gcc version there's some cast to __packed array or something, probably ineffective/harmless in the same way except it does not thow the warning? I don't know don't care about Cube
- ZLPs are useful, I referred to chapter and verse in the fine manuals above
- in Synopsys OTG USB used in higher-end STM32, the "transfer start" and "dump data" (and then also a "done/PC got it") processes are separate to allow replacing the latter by built-in DMA seamlessly
- in that USB, when "transfer start" is with ZLP, it does not throw the "dump data" interrupt at all, so there's no point to discuss zero data case in said "dump data" functions
- the free service is late and lacking and the $14B company does not care
- USB is crap but who are we to judge
- sometimes users in fora are rude to each other

JW

[peter-h]

Thank you.

__packed attribute is already not there in said functions the current/recent incarnations of Cube

Are these online somewhere? I looked in the Cube library (c:\st or c:\users) but the file stm32f4xx_ll_usb.c is not there. Maybe it is compressed into something...

sometimes users in fora are rude to each other

It achieves nothing though.

[wek]

I'm out of office but try googling for CubeF4 github.

JW

[peter-h]

I think I found it here
https://github.com/STMicroelectronics/STM32CubeF4/blob/master/Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_ll_usb.c

and the code there is

Code: [Select]


/**
  * @brief  USB_WritePacket : Writes a packet into the Tx FIFO associated
  *         with the EP/channel
  * @param  USBx  Selected device
  * @param  src   pointer to source buffer
  * @param  ch_ep_num  endpoint or host channel number
  * @param  len  Number of bytes to write
  * @param  dma USB dma enabled or disabled
  *          This parameter can be one of these values:
  *           0 : DMA feature not used
  *           1 : DMA feature used
  * @retval HAL status
  */
HAL_StatusTypeDef USB_WritePacket(USB_OTG_GlobalTypeDef *USBx, uint8_t *src,
                                  uint8_t ch_ep_num, uint16_t len, uint8_t dma)
{
  uint32_t USBx_BASE = (uint32_t)USBx;
  uint8_t *pSrc = src;
  uint32_t count32b;
  uint32_t i;

  if (dma == 0U)
  {
    count32b = ((uint32_t)len + 3U) / 4U;
    for (i = 0U; i < count32b; i++)
    {
      USBx_DFIFO((uint32_t)ch_ep_num) = __UNALIGNED_UINT32_READ(pSrc);
      pSrc++;
      pSrc++;
      pSrc++;
      pSrc++;
    }
  }

  return HAL_OK;
}

/**
  * @brief  USB_ReadPacket : read a packet from the RX FIFO
  * @param  USBx  Selected device
  * @param  dest  source pointer
  * @param  len  Number of bytes to read
  * @retval pointer to destination buffer
  */
void *USB_ReadPacket(USB_OTG_GlobalTypeDef *USBx, uint8_t *dest, uint16_t len)
{
  uint32_t USBx_BASE = (uint32_t)USBx;
  uint8_t *pDest = dest;
  uint32_t pData;
  uint32_t i;
  uint32_t count32b = (uint32_t)len >> 2U;
  uint16_t remaining_bytes = len % 4U;

  for (i = 0U; i < count32b; i++)
  {
    __UNALIGNED_UINT32_WRITE(pDest, USBx_DFIFO(0U));
    pDest++;
    pDest++;
    pDest++;
    pDest++;
  }

  /* When Number of data is not word aligned, read the remaining byte */
  if (remaining_bytes != 0U)
  {
    i = 0U;
    __UNALIGNED_UINT32_WRITE(&pData, USBx_DFIFO(0U));

    do
    {
      *(uint8_t *)pDest = (uint8_t)(pData >> (8U * (uint8_t)(i)));
      i++;
      pDest++;
      remaining_bytes--;
    } while (remaining_bytes != 0U);
  }

  return ((void *)pDest);
}
I can't understand what they are trying to do given the 32F4 supports unaligned 32 bit accesses in hardware.

[ataradov]

I can't understand what they are trying to do given the 32F4 supports unaligned 32 bit accesses in hardware.

The code may be universal for all devises with the same controller, so they want to be as compatible as possible.

[peter-h]

OK. What is this

Code: [Select]

   do
    {
      *(uint8_t *)pDest = (uint8_t)(pData >> (8U * (uint8_t)(i)));
      i++;
      pDest++;
      remaining_bytes--;
    } while (remaining_bytes != 0U);
Isn't it writing 1 byte at a time into the USB FIFO?

[ataradov]

Where is this code coming from? I see it from the quoted code above.

It is not. It reads FIFO value into pData and then copies it byte by byte into the final buffer.

Although pData is always aligned, so this "__UNALIGNED_UINT32_WRITE(&pData, USBx_DFIFO(0U));" is not necessary, it could just be "pData = USBx_DFIFO(0U)".

[peter-h]

Could this entire issue not be solved simply by __align(4) on the tx and rx buffers, and by moving uint32 values to/from the USB controller?

Wek has a point about "zero-copy" buffer applications but do these really exist in USB, in the arm32 context? Much has been made of that in the ETH/LWIP department but actually the perf gain there is approximately minus zero, and I don't think anybody is up to fixing all the bugs in the latest ST ETH drivers.

[ataradov]

Could this entire issue not be solved simply by __align(4) on the tx and rx buffers, and by moving uint32 values to/from the USB controller?

Yes, but you would have to align the buffers at the point of call, and they did not want to place this constraint on their APIs.

In my implementations I do require buffer alignment with no issues and it indeed simplifies everything.

[peter-h]

you would have to align the buffers at the point of call,

No doubt I am missing something but surely when you call this code, the buffer can be either in main RAM or on the stack, and an __align directive will work in either case.

Not sure what happens with a buffer on the heap; no doubt it depends on the heap implementation.

[ataradov]

an __align directive will work in either case.

Yes, but my point is that you as a caller have to ensure alignment of each buffer you pass. You can't just declare function arguments as aligned.  This is not hard to do, but may be you expect users of this API to not remember this in all cases. 

It is like needing to declare a variable shared with an interrupt handler as a volatile. It is a thing you need to do, but people forget about that all the time and run into issues.

So they may have decided to do more work inside the function so that you never have to think about it outside.

[gf]

Not sure what happens with a buffer on the heap; no doubt it depends on the heap implementation.

malloc() returns a pointer that is aligned at least as strictly as max_align_t.
For even stricter aligment requirement you can call aligned_alloc().

[peter-h]

So they may have decided to do more work inside the function so that you never have to think about it outside.

OK; how about defining the buffer as a uint32_t one? Then alignment is implicit, the caller can't forget, but he has to mess with pointers if feeding bytes into it.

malloc() returns a pointer that is aligned at least as strictly as max_align_t.

I can't find it in my source, but that sounds like 8 or even 16, according to google. My biggest scalar is 8. Thank you.

[ataradov]

OK; how about defining the buffer as a uint32_t one? Then alignment is implicit, the caller can't forget, but he has to mess with pointers if feeding bytes into it.

Yes, but now if you naturally want to work with unaligned data, you are going to have the same messy loops all over the code instead of one place in the final function.

It is just a decision you need to make at the architecture stage. If you are ok designing everything around aligned buffers, then there is no issue. If you are not ok with that, then doing it where ST did is the best approach. ST did not want to make this decision for their customer, so they went the safe route.

And of you are writing the drivers yourself, you can make this decision yourself. Otherwise you just have to live with the decision vendor made for you.

[gf]

malloc() returns a pointer that is aligned at least as strictly as max_align_t.

I can't find it in my source, but that sounds like 8 or even 16, according to google. My biggest scalar is 8. Thank you.

It's C11.
alignof(long double) may be 16 on some platforms.