USB failure with gcc-4.9 resolved

Post here first, or if you can't find a relevant section!
Post Reply
User avatar
Slammer
Posts: 241
Joined: Tue Mar 01, 2016 10:35 pm
Location: Athens, Greece

USB failure with gcc-4.9 resolved

Post by Slammer » Mon Apr 18, 2016 10:50 pm

I am trying some hours to resolve the problem with gcc 4.9.
As a reminder the problem is that usb serial is not working after downloading the sketch. In my Debian machine the kernel reports:

Code: Select all

[29741.174860] usb 8-3: new full-speed USB device number 20 using ohci-pci
[29741.347980] usb 8-3: New USB device found, idVendor=1eaf, idProduct=0003
[29741.347985] usb 8-3: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[29741.347988] usb 8-3: Product: Maple 003
[29741.347990] usb 8-3: Manufacturer: LeafLabs
[29741.347992] usb 8-3: SerialNumber: LLM 003
[29742.268466] usb 8-3: USB disconnect, device number 20
[29742.766960] usb 8-3: new full-speed USB device number 21 using ohci-pci
[29742.906951] usb 8-3: device descriptor read/64, error 18
[29743.151006] usb 8-3: device descriptor read/64, error 18                                                         
[29743.390947] usb 8-3: new full-speed USB device number 22 using ohci-pci
[29743.530974] usb 8-3: device descriptor read/64, error 18
[29743.775001] usb 8-3: device descriptor read/64, error 18                                                         
[29744.015002] usb 8-3: new full-speed USB device number 23 using ohci-pci
[29744.423012] usb 8-3: device not accepting address 23, error -32
[29744.559066] usb 8-3: new full-speed USB device number 24 using ohci-pci
[29744.967062] usb 8-3: device not accepting address 24, error -32
[29744.967132] usb usb8-port3: unable to enumerate USB device    
The same code with gcc 4.8 as provided by arduino is working normally.
One solution to use gcc 4.9, is to remove all optimizations compiling the code with flag -O0, but the size of binary code is not acceptable.
Trying to isolate the problem by adding and removing a #pragma GCC optimize ("O0") directive in each file related with usb, I finally found that the problem is in usb_reg_map.h when included by usb_cdcacm.c
I knew that the problem is related with some missing volatile declarations, by studying the files line by line (is a mess actually) I found some very odd functions. For example look the code in this function at line 441 of usb_reg_map.h

Code: Select all

static inline void usb_set_ep_tx_addr(uint8 ep, uint16 addr) {
    uint32 *tx_addr = usb_ep_tx_addr_ptr(ep);     
    *tx_addr = addr & ~0x1;
} 
he *tx_addr takes a value (actually is a pointer) and then the variable takes another value, the optimizer correctly(???) discards the first assignment and the reading of usb register never happens (but the usb requires this read for some reason). To disable this optimization and force the compiler to make the read the *tx_addr must be declared as volatile. For some reason the gcc 4.8 does not optimize this sequence.
The same structure is used by similar functions in lines 457 and 474. By declaring these 3 variables as volatile the sketch creates correctly the usb serial interface. I have switched the default compiler to gcc 4.9 and everything is OK, every code seems to run correctly.

I am not familiar yet with git and I am posting the fix here (forgive me Roger), but the fix is very small, just 3 "volatile" words


PS. Looking the code again I am not sure if the gcc 4.9 operates correctly in this situation..... in the first assignment the pointer is defined and takes a value, in the second assignment the pointer remains the same but the value indexed by pointer is changed. If this is correct the bug is on compiler and not in libmaple.... Ahhh! I am feeling dizzy after so many hours......
Last edited by Slammer on Mon Apr 18, 2016 11:52 pm, edited 3 times in total.

User avatar
Slammer
Posts: 241
Joined: Tue Mar 01, 2016 10:35 pm
Location: Athens, Greece

Re: USB failure with gcc-4.9 resolved

Post by Slammer » Mon Apr 18, 2016 11:46 pm

OK.... After a short study on generated assembly code I am sure that gcc 4.9 correctly optimizes the function, the 'volatile' declaration is needed for the pointer to USB registers.
Either with volatile declaration either without the compiler generates the same code for these two assignments (first assignment gives value to pointer and the second reads the indexed value), but as this function is inlined, actually operates as #define, and the code is always embedded inside the calling function. The optimization alters the code of calling function removing some reads from USB registers but read access from USB registers must always be done with volatile variables!

Read more here: https://github.com/cleanflight/cleanflight/pull/1070

User avatar
RogerClark
Posts: 5936
Joined: Mon Apr 27, 2015 10:36 am
Location: Melbourne, Australia
Contact:

Re: USB failure with gcc-4.9 resolved

Post by RogerClark » Mon Apr 18, 2016 11:50 pm

Hi Slammer

Can you let me know which lines of code need to be changed (have volatile in front of the variable declaration), and I'll change them


Thanks

Roger

User avatar
Slammer
Posts: 241
Joined: Tue Mar 01, 2016 10:35 pm
Location: Athens, Greece

Re: USB failure with gcc-4.9 resolved

Post by Slammer » Tue Apr 19, 2016 12:02 am

Thank you Roger.
I also tested that the volatile declaration of these variables does not change anything with gcc 4.8

Code: Select all

diff --git a/STM32F1/system/libmaple/usb/stm32f1/usb_reg_map.h b/STM32F1/system/libmaple/usb/stm32f1/usb_reg_map.h
index 2e3f6bc..6683264 100644
--- a/STM32F1/system/libmaple/usb/stm32f1/usb_reg_map.h
+++ b/STM32F1/system/libmaple/usb/stm32f1/usb_reg_map.h
@@ -439,7 +439,7 @@ static inline uint16 usb_get_ep_tx_addr(uint8 ep) {
 }
 
 static inline void usb_set_ep_tx_addr(uint8 ep, uint16 addr) {
-    uint32 *tx_addr = usb_ep_tx_addr_ptr(ep);
+    volatile uint32 *tx_addr = usb_ep_tx_addr_ptr(ep);
     *tx_addr = addr & ~0x1;
 }
 
@@ -454,7 +454,7 @@ static inline uint16 usb_get_ep_rx_addr(uint8 ep) {
 }
 
 static inline void usb_set_ep_rx_addr(uint8 ep, uint16 addr) {
-    uint32 *rx_addr = usb_ep_rx_addr_ptr(ep);
+    volatile uint32 *rx_addr = usb_ep_rx_addr_ptr(ep);
     *rx_addr = addr & ~0x1;
 }
 
@@ -471,7 +471,7 @@ static inline uint16 usb_get_ep_tx_count(uint8 ep) {
 }
 
 static inline void usb_set_ep_tx_count(uint8 ep, uint16 count) {
-    uint32 *txc = usb_ep_tx_count_ptr(ep);
+    volatile uint32 *txc = usb_ep_tx_count_ptr(ep);
     *txc = count;
 }

User avatar
RogerClark
Posts: 5936
Joined: Mon Apr 27, 2015 10:36 am
Location: Melbourne, Australia
Contact:

Re: USB failure with gcc-4.9 resolved

Post by RogerClark » Tue Apr 19, 2016 12:09 am

Thanks

I'll try to update it this evening (my time) (around 8 hours from now)

User avatar
martinayotte
Posts: 1171
Joined: Mon Apr 27, 2015 1:45 pm

Re: USB failure with gcc-4.9 resolved

Post by martinayotte » Tue Apr 19, 2016 1:16 am

Thanks for the good catch !
I presume maybe the same bug exist under STM32F4 ? ...
I must admit that "time is a missing ingredient" for me there days ...
Could someone take a quick look if it is ?

User avatar
Slammer
Posts: 241
Joined: Tue Mar 01, 2016 10:35 pm
Location: Athens, Greece

Re: USB failure with gcc-4.9 resolved

Post by Slammer » Tue Apr 19, 2016 1:25 am

It is very possible to have the same bug on STM32F4 as gcc 4.9 is more aggressive with optimizations. Despite of this aggression I noticed a small increase in binary code... not big difference but some bytes in various functions.... :!:

User avatar
RogerClark
Posts: 5936
Joined: Mon Apr 27, 2015 10:36 am
Location: Melbourne, Australia
Contact:

Re: USB failure with gcc-4.9 resolved

Post by RogerClark » Tue Apr 19, 2016 9:46 am

@slammer

I've manually applied you fix and tested it with my Maple mini and Serial USB still seems to work fine. So I've comitted the fix and pushed to the repo

See

https://github.com/rogerclarkmelbourne/ ... its/master

Specifically https://github.com/rogerclarkmelbourne/ ... 7164123545

Re: Possibly issue on the F4.

I don't think it would do any harm (apart from code execution speed) if the same vars were marked as volatile on the F4
(Sorry I'm not in a position to test this on the F4 either)

Thanks

Roger

Post Reply

Who is online

Users browsing this forum: No registered users and 2 guests