introduce a context structure to encompass global state in the K64F Storage driver #2121
Conversation
rgrover
changed the title
|
@simonqhughes could you please confirm that this change set works for you. @sg- @0xc0170 Please help merge this change. It sets up the stage for the next bit of work around solving the read-while-write issue with K64F |
Any update? It looks fine to me |
|
Hi Here are the cfstore test results for a flash-journal sync mode build Here are the results for the async case: |
|
@simonqhughes Could you please elaborate on the failure? I don't expect any of the flash-journal tests would fail as a result of this change. Did this test pass with code prior to this change? Are you mindful of the new CONFIG variable to define START_ADDR and SIZE? |
|
@simonqhughes submitted #454 to fix tests for the synchronous case. The problem was with the tests, not with the Storage driver. |
|
I understand the failure you mention was caused by an out of date version of the flash-journal basicAPI test, which works when the correct version is used. |
| { | ||
| launchCommand(); | ||
|
|
||
| /* At this point, The FTFE reads the command code and performs a series of | ||
| * parameter checks and protection checks, if applicable, which are unique | ||
| * to each command. */ | ||
|
|
||
| if (asyncOperationsEnabled()) { | ||
| /* Asynchronous operation */ | ||
| #if ASYNC_OPS |
There was a problem hiding this comment.
Please explain if you have a strong objection. Otherwise let's avoid loosing time over minor disagreements.
There was a problem hiding this comment.
Please explain if you have a strong objection. Otherwise let's avoid loosing time over minor disagreements.
+1
Not a fan of 14a14a0 but otherwise LGTM
Please provide more details why not a fan and how it could be.
There was a problem hiding this comment.
By authoring a driver you are not in the know of how it will be consumed. By providing 2 methods of operation that are only compile time selectable you cause upstream problems by the consumers of your driver given that multiple consumers may not use the driver in the same way and now you have conflict and nasty failures.
Keep it simple. If you want a sync and async API create both, don't share a common interface and don't make a compile time decision if you support both. Otherwise if there is no consumer for the async API, stash it and remove from the implementation.
There was a problem hiding this comment.
Having multiple compile-time selectable implementations doesn't automatically result in 'upstream problems'. There could be good reasons why users would want to select between the two kinds of behaviors. As the author, I myself have been one of the users of this driver, and I've found it useful to have this option. I may not know all the ways in which the driver would be used, but I do know that both options I provide have their merit.
I agree that keeping it simple might be a deliberate choice. But letting it remain complex and multi-featured doesn't automatically imply problems.
This disagreement is qualitative.
There was a problem hiding this comment.
Having multiple compile-time selectable implementations doesn't automatically result in 'upstream problems'.
It does the moment you have one consumer using sync and the other using async in the same program.
|
Not a fan of 14a14a0 but otherwise LGTM |
Together with other minor changes.
This will bring the state of the K64F driver to the point where we can submit a fix for the read-while-write issue in isolation.
@simonqhughes
follows from #2117