Add support for SST25VFA devices #14

ahogen · 2018-08-21T18:02:21Z

This is kind of an odd part in that it doesn't support JEDEC ID. This class uses some work-arounds to still properly identify the device. The usage is explained in the class's commentary.

NOTE: There are some small changes/additions to the greater library. The significant changes are summarized below.

Include logging library. We (the group I'm with) need logging in almost all areas of a greater Python script we're building. If you (@eblot) disagree with this addition, I can remove it from this PR and use a workaround on my end.
Include random library. Simply used to generate some random data for the chip-test function.
Add erase_chip(). I hoping the implementation makes it easy to add erase_chip() support for the other/existing devices here as well.
(the main event...) Add support for SST25VF010A (1Mbit) and SST25VF512A (512Kbit) devices

I started working off the version I installed from PyPi. The master branch had a few more changes, which were reverted when I copied my mods in. I'm now re-performing those changes.

- Add current SPI speed to print out of SST25VF010A ("__str__()") - Fix "write()" for SST25VF010A - Fix "_unprotect()" for SST25VF010A - Add timings for SST25VF010A - Add "erase_chip()" method

- If device supports chip erase and length of erase is full chip size, perform one chip erase operation instead of multiple sector erase operations. - Many Sst25VF010AFlashDevice methods support Python logging to aid in debugging and just support in general for manufacturing logs. - Sst25VF010AFlashDevice.chip_test() is a new method to exercise the flash by writing, reading, and verifying data on the entire chip. Chip remains filled with the random test data at end of test. Any existing data is permanently lost. - Sst25VF010AFlashDevice.write() returns number of bytes written. Obviously this only applies when the method actually returns. Ive been seen a number of timeouts during writes on this chip, in which case an exception occurs and it's (currently) impossible to tell how far the write progressed before it failed. - Sst25VF010AFlashDevice.write() no longer pops data out of the input array, so external code can still use/inspect the data after the write operation, if desired. - Fix sector size so sector erase operations work correctly for Sst25VF010AFlashDevice

- Added a call to _wait_for_completion() to ensure the device is not busy before sending the first byte. - Added/updated function doc strings with more argument description. - Clean up unnecessary assignment before returning in chip_test()

- Rename Sst25VF010aFlashDevice --> Sst25vfxxxaFlashDevice - Add support for SST25VF512A - Raise exception if chip erase is called by a device which has not explicitly stated that it supports chip erase. - Minor cleanup

ahogen · 2018-08-21T18:06:33Z

spiflash/serialflash.py

@@ -165,7 +167,11 @@ def get_flash_device(url, cs=0, freq=None):
    @staticmethod
    def read_jedec_id(spi):
        """Read flash device JEDEC identifier (3 bytes)"""
-        jedec_cmd = Array('B', (SerialFlashManager.CMD_JEDEC_ID,))
+        jedec_cmd = []
+        if not isinstance(SerialFlashManager.CMD_JEDEC_ID, type(Array('B', [0x0]))):


FYI: This was added so users can override CMD_JEDEC_ID if required.

Update/fix logger name so it shows as a child of the module

- Device was reporting size in bits not bytes. I got confused somehow and thought everything was reporting in bits. Not sure where I got that now... Regardless, fixed, so everything is in bytes. - Only had 4k sector erase capability. Added 32k block erase capability.

Removing extra fluff that I thought might be cool but probably doesn't belong in this library. I didn't get any approval/disapproval from lib owner/maintainer eblot but I thought it might come up and decided to remove it anyway. At this point, I would like to squash all these commits down into one so the PR on github is smaller. I've fixed some bugs in this new class, and that history probably doesn't need to be shown. I'd like to get eblot's oppinion before I squash though.

ahogen · 2018-09-05T00:49:04Z

I mentioned in the last commit message that I'd like to squash all these commits down into one so the PR diff looks prettier. However, I didn't want to do that before @eblot has a chance to say yes/no/wtf or provide any other commentary for which the git history may be a useful resource.

@eblot, let me know what you'd prefer me to do and I'll go ahead and do it. (leave it as it is with full history, squash and make a new PR, change something in the code...)

Cheers!

eblot · 2018-09-05T07:55:44Z

Hi Alex,

Sorry for the long delay to reply.
I'll try to review your changes by today or tomorrow and get you posted. Thanks.

ahogen · 2018-09-05T19:24:05Z

Great! I'm sorry if I made you feel pressured. That was not my intention. I am just working on a project internal to my company and wanted to point to the official PySpiFlash repository soon-ish (1-2 weeks) for this new SPI flash part, instead of my fork.

Looking forward to your comments.

Cheers!

eblot · 2018-09-06T08:49:05Z

No that's ok for polling for news, if not your PR could stay idle for months :-)

Starting reviewing now.

eblot · 2018-09-06T10:41:21Z

SST25VFA proposed changes

Styling

couple of PEP8/pylint improvements
 if do not need extra () (according to pylint)
 indentation, spaces, 80-column rule
doc style: follow pyftdi one  - BTW there is another PR (Adding typehints(breaks python 2.x compatibility), chip_erase, AT25SF041 #8) to add type hints, need to look at it as well :-)
Remove the XX from the flash name (if possible)

Implementation

with if (chiperase): return rather than else & long block indentation
remove verify=False # what was the rationale here?
I do not get self._size == length * 8 : self._size unit is bytes, length is bytes as well, so what length * 8 is for? (looks like bits)
read_jedec_id hack is unfortunately not a good solution, as this means once the class has been loaded and used once, it cannot be reused to detect/access other devices. This breaks the purpose of the SerialFlashManager, i.e. acts as a factory.

I would prefer something like an SerialFlashManager extension.
Obviously it would be better to have a generic SerialFlashManager with a Jedec-compliant and Jedec-less incarnations, but it cannot be achieved without breaking the API, which I’d prefer to avoid - but it could be done later if another major change requires to break it for a strongest reason.

Can you have a look at my sst25vf010-tweaks branch (edited from your PR). I doubt it runs flawlessly as I do not have the HW to even start communicating, but you would get the idea.

eblot · 2018-09-06T14:17:28Z

Note that the rework for #8 may conflict with this PR as well (chip erase stuff, reworked API signature due to type hints).

ahogen · 2018-09-07T20:46:38Z

Great! Thanks for the detailed comments. I'll try and get to this in the next week. Vacation time away from the hardware will add some delays.

It sounds like I should wait for the other PR to merge, and incorporate those changes into this PR. Apologies, I didn't look at any of the open PRs before making my changes/additions.

Since there is a chip_erase() in the other PR, would you like me to remove the erase_chip() from mine? Or vise versa?

I will definitely look at your sst25vf010-tweaks branch when I get to making code changes.

Implimented basically all changes suggested eblot#14 (comment). Cleaned up pep8 and pylint warnings/errors if they were related to new code I was writing. Currently untested, but hope to try out on hardware shortly.

ahogen · 2018-09-19T05:53:49Z

I think I've added all of your suggested changes in. None of it has been tested yet. I'll try it out on hardware hopefully later this week. Been busy at the office so while this will be used at work, I'm currently working on these changes in my off-hours.

In response to your above bullet points:

couple of PEP8/pylint improvements

Whoops. Went ahead and changed most. Not sure how to get rid of pep8 E128 "continuation line under-indented for visual indent" errors though.

 * if do not need extra () (according to pylint)

Done
 

indentation, spaces, 80-column rule

Got it.

doc style: follow pyftdi one  - BTW there is another PR (Adding typehints(breaks python 2.x compatibility), chip_erase, AT25SF041 #8) to add type hints, need to look at it as well :-)

I think I fixed up most of the method doc strings. I'll wait on the class doc string (specifically the usage examples) until the API is settled, since it looks like you've suggested a new flash manager class.

About type hints: Would you like me to wait for the new PR to be merged? Or are you saying you'd just like me to add type hints for methods in this new SPI flash class?

Remove the XX from the flash name (if possible)

You bet.

with if (chiperase): return rather than else & long block indentation

All other erase "sections" (if blocks) erase progressively smaller chunks of memory, provided the chip supports it. The chip erase is unique in that there is no configurable size. I guess I figured that because it is different, and because we're erasing the whole chip, we should never bother to check if the device supports smaller erase sizes. This is probably from my embedded C background where I try to make the number of executed instructions smaller.

But, sure, for code readability, I can just set the start = end; rstart = rend and un-indent the following if statements.

remove verify=False # what was the rationale here?

Eeerrr... I had the erase_chip(verify=verify), so after calling erase_chip(), I set it to false to disable a second verification cycle down below all the if statements. Now that I'm re-reading this, it doesn't make much sense.

Smarter thing is probably for me to keep erase_chip(verify=False) and then let the if verify: further down actually run the verification. Fixed in (untested) commit.

I do not get self._size == length * 8 : self._size unit is bytes, length is bytes as well, so what length * 8 is for? (looks like bits)

I was confused for a while and thought everything was in bits not bytes and had a bunch of x 8 everywhere. I removed most in commit f82be9a, but it looks like this one got through. Oops.

read_jedec_id hack is unfortunately not a good solution, as this means once the class has been loaded and used once, it cannot be reused to detect/access other devices. This breaks the purpose of the SerialFlashManager, i.e. acts as a factory.

That makes sense.

Would you like me to try out your SerialFlashManagerNoJedec concept on hardware, or should I just write the usage documentation in my class to show how to use the Sst25VfaFlashDevice class directly, without a serial flash manager? This is why the flash_id in __init__() defaults to None, so a user can attempt to just use an SSTVFA dev with as little fuss as possible.

ahogen · 2018-10-13T20:07:17Z

I noticed you haven't replied to my questions above, but I wanted to check in to say that we've been using this device class for the past few weeks and haven't had any issues. We're instantiating the device class directly though, instead of using a SerialFlashManager* tool to detect the device.

Since it seems like we're both busy with other things, I feel like I should make this PR as simple as possible so I can get this device added to the library. So I think I would like to remove the SerialFlashManagerNoJedec class as I likely won't have time to develop a test to try this out.

The one change that I would like implemented is the erase_chip method. You mentioned before that PR #8 has a chip_erase added, so I'll ask again: Which implementation do you prefer, the one here in PR #14 or the one in PR #8? I'd be happy to remove mine if I know that PR #8 can be merged in quickly.

So in summary, what I would really like is to get this Sst25vfa class + a chip erase method. Any other changes I may have made I would be happy to revert if that would help this PR to get merged.

Thanks and cheers! 🙂

ahogen · 2018-11-09T17:59:48Z

Expired.

Neither of us have time to get this coordinated with other PRs in the repository and I no longer have a need to use this SPI flash device.

ahogen added 8 commits August 21, 2018 09:35

Initial SST25VF010A add. Not yet fully tweaked or tested.

0646846

Revert unintended changes

f474376

I started working off the version I installed from PyPi. The master branch had a few more changes, which were reverted when I copied my mods in. I'm now re-performing those changes.

Add some usage examples to this device class, since the dev is special

ffd6fdd

Add "erase_chip()". Fixes to SST25VF010A dev

007d999

- Add current SPI speed to print out of SST25VF010A ("__str__()") - Fix "write()" for SST25VF010A - Fix "_unprotect()" for SST25VF010A - Add timings for SST25VF010A - Add "erase_chip()" method

SST25VF010A: Fix comment. Clean up debug code

7e92841

Add support for 512Kbit. Rename class SST25VF010A -> SST25VFxxxA

b07626f

- Rename Sst25VF010aFlashDevice --> Sst25vfxxxaFlashDevice - Add support for SST25VF512A - Raise exception if chip erase is called by a device which has not explicitly stated that it supports chip erase. - Minor cleanup

ahogen commented Aug 21, 2018

View reviewed changes

ahogen added 4 commits August 24, 2018 12:49

SST25VFxxxA: Fix bug where canceled write locks device

d85d031

Update/fix logger name so it shows as a child of the module

Fix checking for chip erase condition

6dea393

SST25VF010A: Add @eblot proposal + cleanup

36cf58d

Implimented basically all changes suggested eblot#14 (comment). Cleaned up pep8 and pylint warnings/errors if they were related to new code I was writing. Currently untested, but hope to try out on hardware shortly.

ahogen changed the title ~~Add support for SST25VFxxxA devices~~ Add support for SST25VFA devices Oct 13, 2018

ahogen closed this Nov 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for SST25VFA devices #14

Add support for SST25VFA devices #14

ahogen commented Aug 21, 2018

ahogen Aug 21, 2018 •

edited

Loading

ahogen commented Sep 5, 2018

eblot commented Sep 5, 2018

ahogen commented Sep 5, 2018 •

edited

Loading

eblot commented Sep 6, 2018

eblot commented Sep 6, 2018

eblot commented Sep 6, 2018

ahogen commented Sep 7, 2018

ahogen commented Sep 19, 2018

ahogen commented Oct 13, 2018

ahogen commented Nov 9, 2018

Add support for SST25VFA devices #14

Add support for SST25VFA devices #14

Conversation

ahogen commented Aug 21, 2018

ahogen Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

ahogen commented Sep 5, 2018

eblot commented Sep 5, 2018

ahogen commented Sep 5, 2018 • edited Loading

eblot commented Sep 6, 2018

eblot commented Sep 6, 2018

SST25VFA proposed changes

Styling

Implementation

eblot commented Sep 6, 2018

ahogen commented Sep 7, 2018

ahogen commented Sep 19, 2018

ahogen commented Oct 13, 2018

ahogen commented Nov 9, 2018

ahogen Aug 21, 2018 •

edited

Loading

ahogen commented Sep 5, 2018 •

edited

Loading