cpu/stm32/usbdev_fs: allow inverted disconnect GPIO#21718
cpu/stm32/usbdev_fs: allow inverted disconnect GPIO#21718maribu merged 1 commit intoRIOT-OS:masterfrom
Conversation
maribu
added
Type: new feature
github-actions
bot
added
Platform: ARM
Did you also test it with a board where the pin is not inverted? One of the upstream boards? I think I have a board with USB peripheral somewhere to test this. |
|
No, I "tested" this by "staring hard at the diff". But I'm having a Bluepill somewhere in reach, I'll give it a spin in a sec. |
| uint8_t irqn; /**< IRQ channel */ | ||
| uint8_t apb; /**< APB bus */ |
There was a problem hiding this comment.
Why did you change the order in the struct? 🤔
There was a problem hiding this comment.
See commit message / PR description:
In addition the members in the struct are sorted by alignment, as this is a foolproof algorithm to prevent wasting memory on unneeded padding. (It does not yield savings here, but it is a good practice.)
There was a problem hiding this comment.
E.g. consider:
#include <stdio.h>
#include <stdint.h>
struct foo1 {
uint8_t a;
uint32_t b;
uint8_t c;
uint16_t d;
};
struct foo2 {
uint32_t b;
uint16_t d;
uint8_t a;
uint8_t c;
};
int main(void)
{
printf("sizeof(struct foo1) = %zu, sizeof(struct foo2) = %zu\n",
sizeof(struct foo1), sizeof(struct foo2));
return 0;
}~ ➜ gcc -Os -o test test.c
~ ➜ ./test
sizeof(struct foo1) = 12, sizeof(struct foo2) = 8
Both structs would be identical for the given use case, but one wastes 4 B of RAM.
There was a problem hiding this comment.
Oh, the gpio_af_t actually can be implemented as uint8_t and sorting that as well. With that sorted in correctly, the struct actually will be smaller than before, despite having one field more :)
There was a problem hiding this comment.
My concern was that some part of the code might be dependant on the order of the struct, but I guess for the internal STM32 configuration structure, there is no wild casting going on from one struct type to another.
There was a problem hiding this comment.
Yes, there would be no reason to cast config structs. One could embed one in another and use container_of() to almost safely cast back to the "outer/child struct".
Just casting between to unrelated structs would be asking for trouble.
There was a problem hiding this comment.
My concern was that some part of the code might be dependant on the order of the struct,
Well 🤣
There was a problem hiding this comment.
Oh man, C++ is a pain in the ass
|
Tested just now: So even re-enumeration after reboot (the important part) does still work. |
crasbe
left a comment
crasbe
left a comment
There was a problem hiding this comment.
I wonder if the file should rather be in the cpu_stm32_usbdev subgroup [3], because currently the entries are not added to that group and the cpu_stm32 page also does not show the content explicitly [1]. You have to nagivate to the file [2], which is not easy to find IMO.
[1] https://ci.riot-os.org/results/76a1ab86808d47e8ba3d70c69f14fcec/doc-preview/group__cpu__stm32.html
[2] https://ci.riot-os.org/results/76a1ab86808d47e8ba3d70c69f14fcec/doc-preview/cpu__usbdev_8h.html
[3] https://ci.riot-os.org/results/76a1ab86808d47e8ba3d70c69f14fcec/doc-preview/group__cpu__stm32__usbdev.html
| #define USBDEV_CPU_DMA_REQUIREMENTS __attribute__((aligned(USBDEV_CPU_DMA_ALIGNMENT))) | ||
|
|
||
| /** | ||
| * @name Flags used in @ref stm32_usbdev_fs_config_t |
There was a problem hiding this comment.
maribu
force-pushed
the
cpu/stm32/usbdev_fs/inverted-disconnect-pin
branch
from
2e0f128 to
3b8fef7
Compare
|
Ah, we could do better having the different two flavors of the USB peripheral more prominently appear in the doc. I'll rework it here soonish |
maribu
force-pushed
the
cpu/stm32/usbdev_fs/inverted-disconnect-pin
branch
from
3b8fef7 to
3e6e965
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
maribu
force-pushed
the
cpu/stm32/usbdev_fs/inverted-disconnect-pin
branch
from
3e6e965 to
b1b2664
Compare
Is that still planned for this PR? |
|
It actually is already distinct, I just didn't track the development close enough to be aware that the synopsis driver has been split out and used by different CPUs. |
|
You can squash and then we try to merge again 🤔 |
The STM32F3 requires a dedicated digital signal to emulate a disconnect event by pulling D+ down via a 1.5 kOhm resistor. Some boards, such as the OLIMEXINO-STM32F3, do not directly connect a GPIO but place a transistor in between. Depending on the exact implementation, the logic level may end up being inverted compared to directly connecting a GPIO. This adds a flag member to the `stm32_usbdev_fs_config_t` and a new flag to indicate inverted logic. In addition the members in the struct are sorted by alignment, as this is a foolproof algorithm to prevent wasting memory on unneeded padding. Finally, the USB driver is adapted to honor the flag. Co-authored-by: crasbe <[email protected]>
maribu
force-pushed
the
cpu/stm32/usbdev_fs/inverted-disconnect-pin
branch
from
64f356d to
26d8fe4
Compare
|
Whoohoo! Time to open a PR that needs this magic 🎉 |
4 participants
Contribution description
The STM32F3 requires a dedicated digital signal to emulate a disconnect event by pulling D+ down via a 1.5 kOhm resistor. Some boards, such as the OLIMEXINO-STM32F3, do not directly connect a GPIO but place a transistor in between. Depending on the exact implementation, the logic level may end up being inverted compared to directly connecting a GPIO.
This adds a flag member to the
stm32_usbdev_fs_config_tand a new flag to indicate inverted logic. In addition the members in the struct are sorted by alignment, as this is a foolproof algorithm to prevent wasting memory on unneeded padding.Finally, the USB driver is adapted to honor the flag.
Testing procedure
No regressions on STM32 boards with the
usbdev_fsUSB peripheral (e.g. Nucleo-F303Ze, Bluepill with STM32F1, p-Nucleo-WB55).We do not have a board upstream that has an inverted disconnect pin yet, but I pinky-promise I tested this with one downstream for which this fixed an issue of USB not re-enumerating on reboot.
Issues/PRs references
None