search

peterhinch / micropython_eeprom

MicroPython device drivers for memory chips (EEPROM, FRAM, Flash, SPIRAM)
MIT License
73 stars 33 forks source link

SPI Flash Driver / Micropython #8

Open mweber-bg opened 3 years ago

mweber-bg commented 3 years ago

Hello Peter, we are developing a new ESP32-based board to monitor and identify disease-causing mosquitoes. On the board I have a Winbond W25Q128JV chip connected to the HSPI pins. I wonder if you could give me some direction how to use your Flash libraries and mount the memory, and if there are any issues I should look out for?

Many thanks in advance,

Michael Weber

peterhinch commented 3 years ago

You are using a chip which hasn't been tested so to some extent you are on your own: I'm not in a position to buy and test every chip on the market. That said, they are fairly standard and the W25Q128JV does support 4K sectors so you're probably in with a good chance. I'm afraid other commitments leave me unable to do a detailed review of the datasheet.

If you follow the docs you should be able to run flash_test.py and littlefs_test.py on the target. These tests are self-documenting: on import they tell you how to run them. You need to run flash_test to create the filesystem before you run littlefs_test. If they run you can be highly confident of your hardware.

Good luck. If you are successful please report back and I will include your chip in the docs.

mweber-bg commented 3 years ago

Hi Peter, Thank you so much for your response! I will follow your advice and let you know the results!

If you are ever bothered by mosquitoes at your home, let me know and I’ll figure out how to get you some mosquito trap gear.

Cheers, Michael

Michael Weber Technology Development

@.*** Weissenburgstr. 22 93055 Regensburg Germany

Mail: @.**@.> Tel.: +1 408-718-8664

www.biogents.comhttp://www.biogents.com

Biogents AG Management Board: Dr. Martin Geier, Dr. Andreas Rose, Dave Avery, Will Golland Chairman of the supervisory board: Jürgen Peter Registered at district court Regensburg, register number HRB 10692

@.***

On Aug 17, 2021, at 8:28 AM, Peter Hinch @.**@.>> wrote:

You are using a chip which hasn't been tested so to some extent you are on your own: I'm not in a position to buy and test every chip on the market. That said, they are fairly standard and the W25Q128JV does support 4K sectors so you're probably in with a good chance. I'm afraid other commitments leave me unable to do a detailed review of the datasheet.

If you follow the docs you should be able to run flash_test.py and littlefs_test.py on the target. These tests are self-documenting: on import they tell you how to run them. You need to run flash_test to create the filesystem before you run littlefs_test. If they run you can be highly confident of your hardware.

Good luck. If you are successful please report back and I will include your chip in the docs.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/peterhinch/micropython_eeprom/issues/8#issuecomment-900029822, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ARYD7D73CECACR55BGDS2VLT5H6QZANCNFSM5CH2W66Q. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email.

mweber-bg commented 3 years ago

Hi Peter, After returning from vacation in Iceland (beautiful!), I got back to connecting the Winbond W25Q128JV flash. Test() and full_test() passed, and fstest(True) also appeared to work but cptest() failed and the flash could no longer be mounted once unmounted. To make a long story short, nothing was actually written because the FLASH class was using 4-byte addresses because the flash size hardwired into the constructor was 4096 and it can be as much as 16384 which is the case with this Winbond flash.

So I made a few changes to the constants and the constructor of the FLASH class (below), and added some constant and comments that might be useful for others. Now everything is working beautifully with my Winbond memory.

Also in the BlockDevice class, I noticed that the ioctl function does not cover the case for op=6: erase block [512 bytes]. In principle it is therefore possible that a block has previously written data in it. Addressing this is fairly straightforward (see revised function at the end).

class BlockDevice:

    def __init__(self, nbits, nchips, chip_size, verbose=True):
        self._c_bytes = chip_size  # Size of chip in bytes
        self._a_bytes = chip_size * nchips  # Size of array
        self._nbits = nbits  # Block size in bits
        self._block_size = 2**nbits
        self._rwbuf = bytearray(1)
        self._zeros = bytearray(self._block_size)
        if verbose:
            print ('Block device created\nChip size: {:d} bytes\nChip count: {:d}\nFile system block size: {:d} ({:d} bits)' \
                   .format(chip_size, nchips, 2**nbits, nbits))

    def ioctl(self, op, arg):  # ioctl calls: see extmod/vfs.h
            # debug
            # print ('ioct: op={}, arg={}'.format(op,arg))
            if op == 1:  # INITIALIZE
                self.initialise()
                return
            if op == 2:  # SHUT DOWN: make sure data are written
                self.sync()
                return
            if op == 3:  # SYNCHRONISE
                self.sync()
                return
            if op == 4:  # BP_IOCTL_SEC_COUNT
                return self._a_bytes >> self._nbits
            if op == 5:  # BP_IOCTL_SEC_SIZE
                return self._block_size
            if op == 6:  # ERASE
                self.writeblocks(arg, self._zeros)
                return 0

# Refer to flash chip datasheet to confirm parameters and commands

# Supported instruction set:

# SPI commands with 3 or 4 byte address
_READ = const(0)
_PP = const(1)
_SE = const(2)
_CMDS3BA = b'\x03\x02\x20'
_CMDS4BA = b'\x13\x12\x21'
_SIZE3BA = const(16384) # maximum number of kb for 3 byte address

# SPI commands with no address
_WREN = const(0x06)  # Write enable
_RDSR1 = const(0x05)  # Read status register 1
_RDID = const(0x9f)  # Read manufacturer ID
_CE = const(0xc7)  # Chip erase (takes minutes)

_PAGE_SIZE = const(256)  # Flash page size
_SEC_SIZE = const(4096)  # Flash sector size 0x1000

# Logical Flash device comprising one or more physical chips sharing an SPI bus.
class FLASH(FlashDevice):

    def __init__(self, spi, cspins, size=None, verbose=True,
                 sec_size=_SEC_SIZE, block_size=9, cmd5=None):
        self._spi = spi
        self._cspins = cspins
        self._ccs = None  # Chip select Pin object for current chip
        self._bufp = bytearray(6)  # instruction + 4 byte address + 1 byte value
        self._mvp = memoryview(self._bufp)  # cost-free slicing
        self._page_size = _PAGE_SIZE  # Write uses specified page size.
        # Defensive code: application should have done the following.
        # Pyboard D 3V3 output may just have been switched on.
        for cs in cspins:  # Deselect all chips
            cs(1)
        time.sleep_ms(1)  # Meet Tpu 300μs

        size = self.scan(verbose, size)  # KiB
        super().__init__(block_size, len(cspins), size * 1024, sec_size)

        # Select the correct command set
        if (cmd5 is None and size <= _SIZE3BA) or (cmd5 == False):
            self._cmds = _CMDS3BA
            self._cmdlen = 4
            print ('Flash SPI command length: 4 bytes')
        else:
            self._cmds = _CMDS4BA
            self._cmdlen = 5
            print ('Flash SPI command length: 5 bytes')

        self.initialise()  # Initially cache sector 0
peterhinch commented 3 years ago

Thank you, I'll consider this over the next few days. If I'm to support block sizes other than 4096 I'll need to buy some chips.

Re ioctl erase I can see that providing it is defensive code but in my testing littlefs never called this. Did you encounter cases where this does occur?

FWIW my code matches the example from @Damien in the docs. I'm trying to save 512 bytes of RAM here...

mweber-bg commented 3 years ago

Thanks Peter…

Just to elaborate, the Littlefs block size is the default 512, the sector size is the default 4096, and the chip block size is the default 256. the key change is the test where it determines whether to use 3 or 4 byte addresses.

Littlefs2 consistently calls erase (op 6) before it starts writing into a different 512 byte block.

The RAM use I think could me mitigated if necessary with multiple shorter writes to the cache.

Best Regards, Michael

Sent from my iPhone

On Sep 5, 2021, at 19:39, Peter Hinch @.***> wrote:



Thank you, I'll consider this over the next few days. If I'm to support block sizes other than 4096 I'll need to buy some chips.

Re ioctl erase I can see that providing it is defensive code but in my testing littlefs never called this. Did you encounter cases where this does occur?

FWIW my code matches the example from @damienhttps://github.com/damien in the docshttps://docs.micropython.org/en/latest/reference/filesystem.html#custom-block-devices. I'm trying to save 512 bytes of RAM here...

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/peterhinch/micropython_eeprom/issues/8#issuecomment-913196295, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ARYD7DZVW6MA4JL3W665O3DUAOTORANCNFSM5CH2W66Q. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

peterhinch commented 3 years ago

Thanks for clarifying.

My initial concern is over erase in ioctl which has the potential of being a nasty bug. However I have doubts for the following reasons.

Your observation that littlefs calls the ioctl erase therefore requires explanation, especially as physical re-writing costs flash endurance. One possibility is that littlefs expects a free block to be empty and will fail if it is not. If this were true I think problems would have emerged on MicroPython.

I haven't found a definitive text but my guess is that it the call to ioctl erase is to provide users with an option for security. A system which leaves un-erased blocks presents an obvious target. So the firmware has been written to enable users to choose whether to implement physical erasure. The test scripts check that all works if that choice is not made.

If that guess proves correct I will adopt the approach of documenting it with commented-out code but leave the ioctl unchanged. Users wishing to prioritise security over flash wear (and RAM use) can implement the change you posted.

If you have any definitive information on this I'd be keen to see it.

mweber-bg commented 3 years ago

Hi Peter,

Great analysis and background! It really helps me understand what’s happening in the code.

I am not sure if security is the factor: It seems op code 6 gets issued right before a 512-byte block is at least partially overwritten, anyway. Also, I don’t think there would be extra flash writes - we are just writing zeros to the RAM cache (your code works really great!)

It is stated here that for littlefs, op=6 must be intercepted: https://docs.micropython.org/en/v1.16/library/uos.html

I actually saw that block 0 and block 1 are only partially written (fewer than 512 bytes), and I have no idea if potential non-zero bytes at the end of those specific blocks have any consequences. With other words, without diving deep into littlefs, I couldn’t really tell if op=6 can be safely ignored, and processing seems to be of low cost.

Finally, what might help people adapt your library to new (flash) devices might be my additional comments in flash_spi.py and the FLASH class constructor. What do you think about those?

Cheers, Michael

On Sep 6, 2021, at 10:54 AM, Peter Hinch @.**@.>> wrote:

Thanks for clarifying.

My initial concern is over erase in ioctl which has the potential of being a nasty bug. However I have doubts for the following reasons.

Your observation that littlefs calls the ioctl erase therefore requires explanation, especially as physical re-writing costs flash endurance. One possibility is that littlefs expects a free block to be empty and will fail if it is not. If this were true I think problems would have emerged on MicroPython.

I haven't found a definitive text but my guess is that it the call to ioctl erase is to provide users with an option for security. A system which leaves un-erased blocks presents an obvious target. So the firmware has been written to enable users to choose whether to implement physical erasure. The test scripts check that all works if that choice is not made.

If that guess proves correct I will adopt the approach of documenting it with commented-out code but leave the ioctl unchanged. Users wishing to prioritise security over flash wear (and RAM use) can implement the change you posted.

If you have any definitive information on this I'd be keen to see it.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/peterhinch/micropython_eeprom/issues/8#issuecomment-913470654, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ARYD7D22HIMJKLQR37UJNQLUAR6TFANCNFSM5CH2W66Q. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

peterhinch commented 3 years ago

It is stated here that for littlefs, op=6 must be intercepted

I agree this is ambiguous. I have raised this PR against the doc. This should prompt the maintainers to clarify the point by accepting the PR or telling me I've got it wrong.

Also, I don’t think there would be extra flash writes - we are just writing zeros to the RAM cache

The modified cache will eventually be written out to the device via this code.

Finally, what might help people adapt your library to new (flash) devices might be my additional comments in flash_spi.py and the FLASH class constructor. What do you think about those?

I'll review this material when we have a definitive answer to the erase issue.

mweber-bg commented 3 years ago

Thank you Peter!

Onboard Flash has many advantages over SD (size, power), so this could be important for others developing IoT applications. —Michael

On Sep 7, 2021, at 9:22 AM, Peter Hinch @.**@.>> wrote:

It is stated here that for littlefs, op=6 must be intercepted

I agree this is ambiguous. I have raised this PR against the dochttps://github.com/micropython/micropython/pull/7755. This should prompt the maintainers to clarify the point by accepting the PR or telling me I've got it wrong.

Also, I don’t think there would be extra flash writes - we are just writing zeros to the RAM cache

The modified cache will eventually be written out to the device via this codehttps://github.com/peterhinch/micropython_eeprom/blob/2792ab31d348160e5ad0578a31fb673e748bdd94/bdevice.py#L133.

Finally, what might help people adapt your library to new (flash) devices might be my additional comments in flash_spi.py and the FLASH class constructor. What do you think about those?

I'll review this material when we have a definitive answer to the erase issue.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/peterhinch/micropython_eeprom/issues/8#issuecomment-914057214, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ARYD7D47ZQQ2ECVWYCY7YQDUAW4UDANCNFSM5CH2W66Q. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

peterhinch commented 3 years ago

Hi Michael, you might be interested in the official response to my PR. I believe that my code is correct because it meets the requirement:

have ioctl(6, block) do nothing, but then writeblocks must check if the block needs erasing before it does the write

I can provide a detailed explanation about how this works if you wish.

My guess about security was evidently nonsense. I have updated my proposed text for the docs.

mweber-bg commented 3 years ago

Thanks Peter for addressing this. I don’t think I need a more detailed description.

You have made a well-designed driver available and I thank your for that.

The Winbond 128 MBit memory can be noted as tested with the library; minimally the constructor in the FLASH class constructor needs to be changed (size needs to be compared with 16384, or use of the 3-byte address SPI command hard-coded. I have also added a couple of comments at the beginning of flash_spi.py and debug printout in BlockDevice constructor to make it easier to test other device.

…Michael

On Sep 8, 2021, at 2:35 PM, Peter Hinch @.**@.>> wrote:

Hi Michael, you might be interested in the official response to my PRhttps://github.com/micropython/micropython/pull/7755. I believe that my code is correct because it meets the requirement:

have ioctl(6, block) do nothing, but then writeblocks must check if the block needs erasing before it does the write

I can provide a detailed explanation about how this works if you wish.

My guess about security was evidently nonsense, and I'll update my text for the docs.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/peterhinch/micropython_eeprom/issues/8#issuecomment-915199726, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ARYD7D3TXL352VHT3FFA24TUA5KALANCNFSM5CH2W66Q. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

peterhinch commented 3 years ago

I will give this some proper attention early next week.

peterhinch commented 3 years ago

I have now had a chance to review this. As you point out, the most significant change is to the Flash class constructor where the 4byte/5byte command set is chosen. I have two issues with your proposed change:

  1. It is a breaking change for users of supported chips.
  2. It seems unnecessary. The cmd5 arg already provides for an override: pass True to force 5 byte, False to force 4 byte. This is documented. Why not just pass False? Have you tried running the driver unchanged with cmd5=False?

The other significant change is to BlockDevice.ioctl in bdevice.py. You add code for calls 1 and 2. I would like to know the circumstances in which these calls occur. Taking initialise first. The FlashDevice.initialise method is called by the Flash constructor. I'm struggling to think of a situation in which ioctl(1) will be called except immediately after the constructor has run (which would harmlessly repeat something already done). Do you know of such a situation?

The case of ioctl(2) is also puzzling. It seems to imply littlefs having prior knowledge of a shutdown. On a microcontroller running on bare metal prior knowledge of a shutdown is a luxury. In cases where it occurs - e.g. there is some kind of physical shutdown button - the system should be designed so that the first thing to know about it is the application. This needs to initiate a shutdown procedure. Typically this would do the following in this order:

  1. Close all files.
  2. Issue sync.

  3. Physically shut down the power.

    In this scenario I can't envisage ioctl(2) occurring: responsibility for issuing sync at the correct time is on the application. Again, if you can enlighten me I'll happily include the code.

All this may seem like nitpicking, but I only include code if I know its exact purpose. I am also highly reluctant to break users' code unless it is the only way to fix a bug.

Incidentally, re Iceland, I've always wanted to go there...

mweber-bg commented 3 years ago

Hi Peter:

You are of course correct, setting cmd5 to False does work. I just didn’t realize that the automatic determination of the command set had to be overridden. I first used just the flash test functions and took the driver as is and insidiously, it seemed to work. It just never actually wrote to the memory and it took me a while to figure out the problem. The only reason I’m saying this is that as flash chips get bigger, somebody else might run into this and maybe some additional notes would be helpful. I’d be happy to put them together.

The ioctl changes shouldn’t be necessary, indeed. Again, I was just trying to figure out if that had something to do with my issue. I’ll take them out.

If you have a chance to go to island - go! There is so much to see and do, including visiting the erupting volcano near the airport: https://volcanoweather.is/9191/2021/09/12

Last not least, a quick question on acquiring sensor data continuously until a threshold is exceeded. I am filling a circular buffer and when a sample exceeds the threshold, I keep a few ms of data before the trigger and sample the next 50 ms worth of data. Then the buffer is written to a flash file (sampling is stopped for that)

The sampling rate is 4 kHz. I am using a timer function and inside the ISR I read the ADC and call the schedule() function.

It appears some process interferes with this scheme because several times a second an error is thrown about the schedule() queue overflowing. Even when I go down to 1 kHz, this happens. So this process must be taking at least 1 ms. I suspect it may be garbage collection but I am not sure.

I saw that you had posted before on how to properly set up a timer and use schedule(). Any advice / tips?

Best Regards, Michael

Sent from my iPhone

On Sep 11, 2021, at 19:21, Peter Hinch @.***> wrote:



I have now had a chance to review this. As you point out, the most significant change is to the Flash class constructor where the 4byte/5byte command set is chosen. I have two issues with your proposed change:

  1. It is a breaking change for users of supported chips.
  2. It seems unnecessary. The cmd5 arg already provides for an override: pass True to force 5 byte, False to force 4 byte. This is documented. Why not just pass False? Have you tried running the driver unchanged with cmd5=False?

The other significant change is to BlockDevice.ioctl in bdevice.py. You add code for calls 1 and 2. I would like to know the circumstances in which these calls occur. Taking initialise first. The FlashDevice.initialise method is called by the Flash constructor. I'm struggling to think of a situation in which ioctl(1) will be called except immediately after the constructor has run (which would harmlessly repeat something already done). Do you know of such a situation?

The case of ioctl(2) is also puzzling. It seems to imply littlefs having prior knowledge of a shutdown. On a microcontroller running on bare metal prior knowledge of a shutdown is a luxury. In cases where it occurs - e.g. there is some kind of physical shutdown button - the system should be designed so that the first thing to know about it is the application. This needs to initiate a shutdown procedure. Typically this would do the following in this order:

  1. Close all files.
  2. Issue sync.
  3. Physically shut down the power.

In this scenario I can't envisage ioctl(2) occurring: responsibility for issuing sync at the correct time is on the application. Again, if you can enlighten me I'll happily include the code.

All this may seem like nitpicking, but I only include code if I know its exact purpose. I am also highly reluctant to break users' code unless it is the only way to fix a bug.

Incidentally, re Iceland, I've always wanted to go there...

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/peterhinch/micropython_eeprom/issues/8#issuecomment-917442060, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ARYD7DZEQST5CT4WEW7LJZDUBOFZJANCNFSM5CH2W66Q. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

peterhinch commented 3 years ago

By all means write some notes on your practical experience. The best destination may be in this developer doc - I could add a new section, and update the link to it in the main README to indicate that it also includes implementation notes. I will of course credit you as author.

Re schedule() this won't work at 4KHz. This is because if a hard ISR runs when a GC is in progress, schedule() is delayed until GC has completed. GC can take anything from 1ms upward. On an ESP32 with SPIRAM 100ms is a regular occurrence. On a normal ESP32 1-10ms is typical, but it depends on your application.

To run at 4KHz (on any current platform) you must do everything that needs that speed in a hard ISR, with all the constraints that implies.