Cleanest way to block 32F417 USB interrupts?

[peter-h]

I have a 4MB serial flash - Adesto SPI, using SPI2 on the 32F417.

Code was generated / extracted from Cube ST libraries to implement a removable storage device via USB, in 2MB of this flash device. This works great. It uses interrupts.

However, other stuff is also accessing this serial flash. The 2MB file system is also accessed via FatFS and that also works, so files can be exchanged between embedded code and say a Windows computer via USB. And other parts are used as a "linear flash" for data logging etc.

I have started doing thorough tests on the data logging code and it runs for a bit and then spectacularly crashes. The stack dump in Cube shows a ton of total crap. Almost certainly the reason is that the USB interrupts are getting in at any time, and both SPI and the flash device itself are obviously not re-entrant

A lot of the serial flash access code is running in main() before USB interrupts are enabled, which is why this issue has only just now surfaced - a year after starting the project. But now I am running test code as an RTOS thread so it exposes the problem.

I am using mutexes to protect SPI and the flash device on the end of it, but this merely enables FatFS and data logging calls to be done in multiple RTOS threads. One cannot use a mutex to block an ISR.

The obvious hack is to block USB interrupts around the SPI2 functions. These functions are called both before and after USB interrupts are enabled, so the usual way is to save the interrupt enable status, disable ints, and restore the status upon exit.

The Q is where. The USB code, done by someone else a couple of years ago, is so vastly complex nobody understands it. But I found these snippets, in which just two lines are interesting:

Code: [Select]

HAL_StatusTypeDef HAL_PCD_Start(PCD_HandleTypeDef *hpcd)
{
#if defined (USB_OTG_FS) || defined (USB_OTG_HS)
  USB_OTG_GlobalTypeDef *USBx = hpcd->Instance;
#endif /* defined (USB_OTG_FS) || defined (USB_OTG_HS) */

  __HAL_LOCK(hpcd);
#if defined (USB_OTG_FS) || defined (USB_OTG_HS)
  if ((hpcd->Init.battery_charging_enable == 1U) &&
      (hpcd->Init.phy_itface != USB_OTG_ULPI_PHY))
  {
    /* Enable USB Transceiver */
    USBx->GCCFG |= USB_OTG_GCCFG_PWRDWN;
  }
#endif /* defined (USB_OTG_FS) || defined (USB_OTG_HS) */
  (void)USB_DevConnect(hpcd->Instance);
  __HAL_PCD_ENABLE(hpcd);
  __HAL_UNLOCK(hpcd);
  return HAL_OK;
}

/**
  * @brief  Stop the USB device.
  * @param  hpcd PCD handle
  * @retval HAL status
  */
HAL_StatusTypeDef HAL_PCD_Stop(PCD_HandleTypeDef *hpcd)
{
  __HAL_LOCK(hpcd);
  __HAL_PCD_DISABLE(hpcd);

  if (USB_StopDevice(hpcd->Instance) != HAL_OK)
  {
    __HAL_UNLOCK(hpcd);
    return HAL_ERROR;
  }

  (void)USB_DevDisconnect(hpcd->Instance);
  __HAL_UNLOCK(hpcd);

  return HAL_OK;
}

__HAL_PCD_ENABLE(hpcd) ;
__HAL_PCD_DISABLE(hpcd) ;

and these are macros and it's obvious what they do:

USBx->GAHBCFG |= 1;
USBx->GAHBCFG &= ~1;

Looking in the RM, page 1274, I see this is toggling a global USB interrupt enable bit, but the description (int pending presentation to application) puzzles me. Is it the right one?

Stepping through the code to see what USBx should be, I see 0x50000000. It is also defined as USB_OTG_FS_PERIPH_BASE. So the way to obtain that bit 0 value is

uint32_t temp = USB_OTG_FS_PERIPH_BASE->GAHBCFG;  // bit 0 holds current int enable status - wrong syntax though; this works

#define USB_CF *(volatile uint32_t*) (USB_OTG_FS_PERIPH_BASE+GAHBCFG)
uint32_t temp=USB_CF

Does this make sense?

Thank you very much for any pointers.

There are more cunning ways to do this, as always, e.g. getting the USB ISR to save a write or read request which is then processed by an RTOS thread, but this should work It is not a high performance USB application.

[DC1MC]

IMHO, blocking the USB interrupts is a sure way to get USB errors, of course one can use such a "Pfuscherei" solution, but as long as you have the use case of simultaneous flash access. you'll have to deal with it sooner or later.

The proper way to do it is to create a "device driver", that is, a process that exclusively access the flash and the USB and local logging sub-system talk with this process and not directly with the flash. Of course, you can also make the USB access have higher priority, because is relatively time critical. With a RTOS is not such a big deal, there are even some implementation floating around, bare metal is a bit of a challenge.


Cheers,
DC1MC

[voltsandjolts]

I don't see why you would need to block interrupts, that would just break the USB link anyway.

The USB host sends IN requests for data, and you NAK until you are ready, then respond with the data.
As long as you are within the 100ms (or whatever) of the request timeout it's all good.
Similarly for OUT, host keeps trying until you acknowledge receipt, within request timeout.

So, USB timing is very forgiving in that respect.
As long as the other processes accessing FLASH don't lock it for tens/hundreds of ms (i.e. more than the USB request timeout) it should be fine.

[peter-h]

The problem is that the USB ISR calls the SPI2 code which then talks to the serial flash - all inside the ISR.

So if there is a foreground task (in this case an RTOS task) accessing the serial flash, that gets interrupted and if the timing is "just right" the hardware itself gets screwed-up because you can't have "SPI within SPI", obviously.

The hack I proposed above has solved the crashing. Right now it seems unbreakable. I can have test code writing (random numbers) solidly into the flash, and writing a 1MB jpeg to the block device and then reboot the target and read it back and it is fine. But I am getting some artefacts on debugs coming out via the USB VCP, which is something else... that code has always been a bit flakey.

My reading of that RM page is that setting that bit to 0 blocks presentation of the interrupt pending to the ISR, and doesn't bugger up the USB subsystem itself. What that exactly does I don't know; I know almost nothing about USB other than it is a master/slave system.

[voltsandjolts]

The problem is that the USB ISR calls the SPI2 code which then talks to the serial flash - all inside the ISR.

Ahh, yes, there's the problem then.
You are effectively demanding immediate access to the SPI FLASH which ain't gonna happen every time.
That request needs to be handed off to a background task.

[mac.6]

If you use an RTOS, you should not doing more in ISR than ack the interrupt and send a event to a task for further processing.
Only very demanding realtime processing can be done under ISR.

[peter-h]

That is quite complicated, not least because I am accessing the serial flash well before the RTOS is running.

In fact I need to access the file system (FatFS) before the RTOS is running, which is possible but it slightly compromises the FatFS features e.g. the file system cannot be re-entrant without an RTOS, but the solution to that is easy (mutex).

There is also a boot loader which accesses it...

So I would need a queuing process where the serial flash is not accessed directly by anything, but that process has to function before the RTOS starts up.

Sure it could be done (e.g. two paths to the serial flash, one used before RTOS starts and another afterwards). Then spend a few days testing it

Disabling USB interrupts around flash r/w is much simpler. For reading the flash, they are disabled for around 500us. For writing the flash (512 byte sector) they are disabled for about 20ms.

However, writing the flash via USB is rare; it is basically dropping in a firmware update or a config file. Reading it is far more common; Windows is continuously polling USB removable devices for all kinds of stuff.

The other thing is that when an embedded app is accessing the serial flash, via FatFS (file system) or for data logging. Then USB reads are also disabled, potentially for 20ms, if the embedded side is writing the flash. So basically USB may be unresponsive for up to 20ms.

I am hoping someone knows the specifics of the USB subsystem, not the "ideal textbook" way of solving this

[voltsandjolts]

Well, that all sounds like day-one architecture problems.

I am hoping someone knows the specifics of the USB subsystem, not the "ideal textbook" way of solving this

The better solution for all involved would be if you spent some time learning (at least the fundamentals of) USB interfacing.
Rather surprised you didn't do that prior to starting a thread asking about USB.

[peter-h]

"The better solution for all involved", old chum, would be IF you knew something useful, you consider posting it, because it might help somebody

Masking that bit does work very well to stop the crashing but it produces occassional corruption of files written to the serial flash removable block device, via USB.

Maybe the text in the RM
Do not change this
register after the initial programming. The application must program this register before
starting any transactions on either the AHB or the USB.
is a clue - even though ST provide a function which does more or less exactly what I am doing.

So I am looking at other ways to disable USB interrupts. There is GINTMSK which can be used to mask all the USB interrupts, so I am setting that to zero (the reset value; all USB ints masked) around the serial flash access functions (one has to do only page read and page write; the USB block device doesn't use any others). That works considerably better but I get a USB error (the block device disappears) when writing large files via USB.

Probably, the loss of USB activity for a solid succession of 20ms slots is enough to muck it up. So, in my test code which is writing solidly to the serial flash I put in a 10ms delay after each page, which should be insignificant (30ms total page write versus 20ms) and this seems to have fixed the USB breakages. In fact I probably should stick a few ms delay after each USB write too, to give the RTOS time to do other stuff.

The USB subsystem in the 32F4 is immensely complex but IMHO the interrupts should work like any other interrupts. I will test other values to see how much breaks it, although it is pointless because one is just hanging up in an ISR.

I would agree it is dumb to be doing a 20ms flash write inside an ISR (especially as one cannot, for various reasons, make the entire operation non-blocking) but this is the code I have, from an ST library, and sorting that would be quite a lot of work.

[voltsandjolts]

"The better solution for all involved", old chum, would be IF you knew something useful, you consider posting it, because it might help somebody

I did, but your ignoring it. Let me try putting it another way:
Disabling USB interrupts is a really shit idea.
Fix your architecture.

[Deni]

You should also be aware that your local file system (FatFS) is not aware of another "user" of your flash storage - so, you can easily end-up with corrupted FAT table or inconsistent reads on the USB host side. USB MSC device acess your drive on block basis - that is, USB host takes care of the FAT file system on your local flash storage and FatFS has no idea that there's somebody else playing in it's playground too. You can somewhat put it under control using semaphores but if host also writes to the flash storage, then the only way is to temporarily unmount FatFS volume while host is attached. That's what I did on a similar project. It was acceptable, since when host was attached there were no "local" actions that would require flash disk access.

[peter-h]

Yes; absolutely. There is no trivial way around that. One approach to detection of USB host file activity is to monitor the USB traffic and look for specific packets. I can't remember but there is a specific packet which tells you the final FAT update was done.  This may be OS specific. Another is using a timeout but that doesn't work with FAT12 (see below). In the opposite direction, FatFS completing the writing of a file can be notified to USB host by doing an unmount/mount, like you say.

Internally, only FatFS is playing in the 2MB FAT12 filesystem, and that is mutex protected to be thread-safe.

There is a bastard of an issue with testing for file corruption: Windows, even latest, has a bug in that FAT12 volumes (anything below 4MB, IIRC) are not write-finalised until you "safely remove hardware". It's like win95 They fixed it for FAT16 etc some years ago.

[peter-h]

Is there some other method to delay a USB operation, involving returning an "error, please retry" packet to the USB host?

[cgroen]

Would MTP be another solution ? Then the disk will not be mounted by Windows but rather it will access it using "commands" which you can handle in the device, this will make the two systems get along nicer...

[Deni]

Yes, that would definitely be a better way, however MTP drivers are not so common as MSC. I think ST's Cube now supports it.

[peter-h]

Sadly it needs to be a standard USB removable device, like a "flash stick". The main use of this feature is for very occassional dropping of config files, firmware updates, etc. With a FAT file system and with the 100k write endurance of the flash (AT45DB321) one can't use it for anything else anyway.

Where I am at currently is this

- r/w to the flash from "inside" (data logging to the separate area) is rock solid, regardless of USB activity (this is a big "win" of the GINTMSK method, although the GAHBCFG (bit 0) method also achieved this)

- USB reads from the flash are rock solid

- USB writes to the flash are 100% solid if there is no inside activity

- USB writes to the flash are 99.9999% solid if the data logging w/r test is running

Obviously the last one is very hard to debug. I am seeing maybe a few bytes off on a 1MB file. I am writing, via USB, 1-2MB jpegs, on which corruption is instantly obvious.

The reliability of the USB writes approaches 100% (so closely it takes hours to get an issue, and in practice nobody will ever see an issue) if I put a 20ms delay in the data logging test code, so each block write (20ms) is followed by a 20ms delay. Some delay is needed anyway otherwise if the data logging writing was solid on the flash (unlikely but possible) then USB flash ops run extremely slowly.

My feeling is that this is something simple. I can't rule out an issue in the PC-USB interface. It could be something like the write to GINTMSK is affecting something, not just simply masking the interrupts. I've looked at e.g. the USB int happening right as the write to GINTMSK is made, but that should still work ok.

All I am currently doing is this:

Code: [Select]

// Register for disabling USB interrupts around serial FLASH r/w
//#define USB_OTG_FS_PERIPH_BASE 0x50000000 - already defined via stm32f4xx_hal.h
//#define GAHBCFG 8 - not used (bit 0 was a potential alternative)
#define GINTMSK 0x18
#define USB_CF *(volatile uint32_t*) (USB_OTG_FS_PERIPH_BASE+GINTMSK)

Code: [Select]

#ifdef USB_INT_PROTECT
// Disable USB interrupts, if they are enabled here
uint32_t temp = USB_CF;  // bit 0 holds current int enable status
USB_CF = 0;
#endif

flash_wait_busy();   // this is blocking, until any previous flash op finishes
flash_cs(0);
flash_writepage();
        flash_cs(1);

#ifdef USB_INT_PROTECT
// Restore previous USB int status
USB_CF = temp;
#endif
I did try making the exit blocking also. Should of course make no difference, and indeed it doesn't.

On top of this I am having to do the testing really carefully, to "safely remove" the device, and not reboot the target while the data logging activity is running in case that code is crapping over the filespace.

It gets better because I am also sending debugs to the PC (teraterm) via a USB VCP, and any issue there could be corrupting USB data, although USB packets are supposed to be CRC checked??

I am also suspecting that if you write a file via USB to the flash, and read it back, that readback is instant because it comes from the Windows cache, so it is meaningless for data comparison reasons, but the act of that readback does "something" to finalise pending writes. It would kind of make sense...

[gf]

There is a bastard of an issue with testing for file corruption: Windows, even latest, has a bug in that FAT12 volumes (anything below 4MB, IIRC) are not write-finalised until you "safely remove hardware". It's like win95 They fixed it for FAT16 etc some years ago.

My understanding is that Quick Removal disables only write caching, but read caching can still happen, and the cached data can be retained (and not read again from the drive) until the filesystem is unmounted (or in other words "savely removed"). Subsequent writes from the host may depend on the cached data (for instance when FAT or directory are updated), but the cached data may no longer be identical to the on-disk data then if the drive was written locally in the meantime. So don't expect data consistency if you write to the drive locally as long as it is mounted by the host. It is an exclusive or, either the drive is mounted locally, or by the host, but not both simultaneously.

I could imagine hacks to kick the host off from the filesystem (when it is mounted by the host). But the consequence is that a (well-behaved) filesystem driver on the host can only prevent further access to the mounted filesystem then, until it gets unmounted (or remounted). But this may not be desired either. [ For instance, if a storage device identifies itself as "removable media" device (RMB bit in the INQUIRY data), then it can signal a media change to the host by answering (rejecting) the next SCSI command with CHECK CONDITION, and returning sense code UNIT ATTENTION and additional sense code MEDIUM MAY HAVE CHANGED in the response to the hosts's subsequent REQUEST SENSE command. The filesystem driver on the host is then expected to react to the signalled media change then. ]

If simultaneous access is desired, I'd rather consider MTP, too, as others have suggested.

[abyrvalg]

Simpsons Android already did it. Before migration to MTP/PTP there was a big button “allow data access” suspending all app activities until unplug. Even if you solve the interrupts at wrong moments, the FS inconsistency problem will remain.

[peter-h]

I am testing the GAHBCFG method (setting bit 0 to 0 - which I earlier abandoned because of occassional errors) and it appears to work identically in the 99.9999% case

It thus seems to be the same as the GINTMSK method, where I was loading 0 into all the bits, to mask all the interrupts.

This is what I would expect, except the RM contains dark warnings about GAHBCFG as posted above.

In fact, last hour of testing, and doing the device removal ultra diligently, I can't break it, over quite a few MB of files written over USB.

The downside of this int disable method is that it also clobbers the USB VCP used for debugs. That isn't important, and I found that if after each internal flash page write (20ms) I have a delay of around 12ms+, the VCP works error-free.

A better way seems to be to get the USB ISR to return an error code (based on an "internal flash in use" flag being set) and then the USB Host should retry. I posted that further back above. It is some SCSI error code. This is the USB ISR:

Code: [Select]

/**
  * @brief  .
  * @param  lun: .
  * @retval USBD_OK if all operations are OK else USBD_FAIL
  *
  */
int8_t STORAGE_Write_FS(uint8_t lun, uint8_t *buf, uint32_t blk_addr, uint16_t blk_len)
{

if (FatFS_filesystem_mounted) return USBD_BUSY;
WritePage(buf, blk_len * STORAGE_BLK_SIZ, blk_addr * STORAGE_BLK_SIZ);
return (USBD_OK);
 
}
I had not noticed the bit which returns USBD_BUSY and that is probably the key.

But using this method doesn't work. Returning USBD_BUSY if an internal flash op is running does work to protect the flash, but any files written via USB are heavily corrupted even if no internal flash ops are running.

BUSY seems the right code because that is how one implements say a USB to serial converter; if the UART TX is full, you return BUSY.

[gf]

As said, I won't be sufficient to trigger the host to retry the SCSI command. This alone won't ensure data consistency. If you mount the file system locally, then you need to ensure that the hosts discards all cached information about the filesystem (for instance by pretending that the flash disk were a removable media device, and telling the host via SCSI sense keys that the medium was removed or changed).

[peter-h]

That's true, but it isn't a problem. The main issue is how to keep USB out of the flash r/w functions when another process is using the flash.

I originally solved it pretty well by disabling USB interrupts. The 2 places one can do that, in addition to doing it in the NVIC which is probably the best way because it avoids hardware related latencies. The main issue with doing this is that it screws up other USB functions like e.g. running debugs over a USB VCP, if there is heavy flash access (not really a big problem though).

What should be better is using the USBD_BUSY return code but today we found a bug in the ST code, in the USB SCSI layer which supports only codes of 0 and negative numbers, while the USB funcs return 0,1,2,3 The bug was reported here
https://community.st.com/s/question/0D53W00001Ie4WcSAJ/32f417-how-to-block-usbfs-isr-from-a-shared-resource-serial-flash-on-spi?t=1641814294067
It has been fixed today and it is working in that USB now keeps out of the flash in a well behaved manner.

Unfortunately something else has got broken which I am trying to fix and which exists only when some flash test code (writing into another part of the flash) is running... Currently I think that the USB host (Windows) simply doesn't like a USBD_BUSY status returned for as long as 20ms (the single flash page write time). The host retries when it gets this code; I don't know how often but probably at (of the order of) 1kHz. Whereas the host didn't mind at all getting nothing back for 20ms and then get a USBD_OK  Well, the obvious answer to this is to farm the USB write request to an RTOS thread which executes it and returns USBD_OK at the end. I have never done FreeRTOS threads which are thus triggered and which totally terminate, so need to look it up.

[nctnico]

I am testing the GAHBCFG method (setting bit 0 to 0 - which I earlier abandoned because of occassional errors) and it appears to work identically in the 99.9999% case

It thus seems to be the same as the GINTMSK method, where I was loading 0 into all the bits, to mask all the interrupts.

This is what I would expect, except the RM contains dark warnings about GAHBCFG as posted above.

In fact, last hour of testing, and doing the device removal ultra diligently, I can't break it, over quite a few MB of files written over USB.

The downside of this int disable method is that it also clobbers the USB VCP used for debugs. That isn't important, and I found that if after each internal flash page write (20ms) I have a delay of around 12ms+, the VCP works error-free.

A better way seems to be to get the USB ISR to return an error code (based on an "internal flash in use" flag being set) and then the USB Host should retry. I posted that further back above. It is some SCSI error code. This is the USB ISR:

Code: [Select]

/**
  * @brief  .
  * @param  lun: .
  * @retval USBD_OK if all operations are OK else USBD_FAIL
  *
  */
int8_t STORAGE_Write_FS(uint8_t lun, uint8_t *buf, uint32_t blk_addr, uint16_t blk_len)
{

if (FatFS_filesystem_mounted) return USBD_BUSY;
WritePage(buf, blk_len * STORAGE_BLK_SIZ, blk_addr * STORAGE_BLK_SIZ);
return (USBD_OK);
 
}
I had not noticed the bit which returns USBD_BUSY and that is probably the key.

This function seems like the perfect place to put the data into a FreeRTOS queue and process it in user space (using regular SPI mutex to lock the SPI interface).

[peter-h]

It should not be necessary; the "proper way" is to return BUSY so the USB Host retries. That AIUI is how USB (a master/slave system) is supposed to work.

No matter how one shakes this, the USB host will be trying to send data much faster than it can be written into flash, so all one can do is offload just the one sector, and after that one has to do "something", which will presumably be USBD_OK, delayed by exactly the same amount (18ms) as the USBD_OK delayed by disabling USB interrupts. So as I see it, the main benefit of offloading the write to an RTOS task is to enable all the other USB functions to work, but they "work" anyway for the same reasons provided one doesn't go too crazy with how long ints are disabled for. I found, experimentally, that USB VCP works if there is a >12ms gap between flash writes, and requiring that isn't a problem.

[bson]

The NVIC supports interrupt priority levels (IPL).  You can make USB interrupts lower priority so they won't pre-empt more important work.  For USB I prefer to make them signal a condition variable which wakes a service thread, then handle all the servicing there.  (Same as with ethernet or ESP32 modules.) There are some hard time limits for e.g. handling suspend and resume, but they're pretty lax and these processors are fast, even at relatively low clocks like 48MHz or whatever.

[SiliconWizard]

The NVIC supports interrupt priority levels (IPL).  You can make USB interrupts lower priority so they won't pre-empt more important work.

Sure, but to be fair, that would be no different from just disabling the USB interrupts while accessing the Flash for other purposes.

Ideally, flash access would be handled at a single point, called either from the USB "driver" or the rest of the code, so you can more easily control shared access. That's the preferred way of implementing shared peripheral resources IMHO. Not sure what the OP uses for USB, but that may imply significant changes in third-party code, something that isn't always pretty to do.