Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

SPI Flash Driver / Micropython #8

Open
mweber-bg opened this issue Aug 16, 2021 · 15 comments
Open

SPI Flash Driver / Micropython #8

mweber-bg opened this issue Aug 16, 2021 · 15 comments

Comments

@mweber-bg
Copy link

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
Copy link
Owner

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
Copy link
Author

mweber-bg commented Aug 17, 2021 via email

@mweber-bg
Copy link
Author

mweber-bg commented Sep 5, 2021 •
edited
Loading

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
Copy link
Owner

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
Copy link
Author

mweber-bg commented Sep 5, 2021 via email

@peterhinch
Copy link
Owner

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.

  • I have tested this pretty thoroughly with long-running torture tests, and no user has yet reported a problem.
  • The example code cited above and MicroPython test scripts such as this one use the same approach as my code. @dpgeorge implemented littlefs on MicroPython. I haven't delved into the C but if his test scripts don't erase I'm confident the release firmware won't.
  • My (limited) understanding of how littlefs works is that de-allocated blocks are simply "dropped" i.e. mapped out and the allocator uses the filesystem data structure to determine whether a block is free. The littlefs docs talk of dropping a block being a no-op.

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
Copy link
Author

mweber-bg commented Sep 6, 2021 via email

@peterhinch peterhinch mentioned this issue Sep 7, 2021
Closed
@peterhinch
Copy link
Owner

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
Copy link
Author

mweber-bg commented Sep 7, 2021 via email

@peterhinch
Copy link
Owner

peterhinch commented Sep 8, 2021 •
edited
Loading

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
Copy link
Author

mweber-bg commented Sep 8, 2021 via email

@peterhinch
Copy link
Owner

I will give this some proper attention early next week.

@peterhinch
Copy link
Owner

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
Copy link
Author

mweber-bg commented Sep 12, 2021 via email

@peterhinch
Copy link
Owner

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.

@peterhinch peterhinch mentioned this issue Jun 21, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@peterhinch @mweber-bg