Add support for SST25VFA devices

[ahogen]

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

[ahogen]

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]

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]

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]

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

Starting reviewing now.

[eblot]

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 (#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]

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

[ahogen]

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.

[ahogen]

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 (#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 https://github.com/eblot/pyspiflash/commit/f82be9a9c584908a5048c02cfcce4d89153f4a3d, 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]

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! :slightly_smiling_face:

[ahogen]

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.