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.

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

Flash interface #2239

Closed
wants to merge 18 commits into from
Closed

Flash interface #2239

wants to merge 18 commits into from

Conversation

d00616
Copy link
Contributor

@d00616 d00616 commented Jan 3, 2015

Here is my actual work on an interface to flash storage for discussion. I have added a simulation. Persistent storage can be added by give a file name via "-F" command line option.

I have no plan to support EEPROM here. At AVR MCU EEPROM cannot accessed directly via memory access. I haven't found any other MCU with EEPROM. It looks like most actual MCU have only flash storage.

My next step is commit support for nRF51 MCU.

When this finished i plan to implement an persistent configuration storage taking care of limited write cycles of flash memory. I plant to put it into this PR.

@LudwigKnuepfer LudwigKnuepfer self-assigned this Jan 4, 2015
@LudwigKnuepfer LudwigKnuepfer added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: drivers Area: Device drivers labels Jan 4, 2015
@LudwigKnuepfer
Copy link
Member

I'll open a PR against your branch with the native specific review.

Please split the interface, test, native, and the preliminary nrf additions into separate commits.


int main(void)
{
// Signature with space for alignment
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use /* comment style */

@d00616
Copy link
Contributor Author

d00616 commented Jan 4, 2015

I plan change flash_get_page_number and flash_get_address to give an return code to add an address check or give back NULL if requested page outside flash area.

@LudwigOrtmann I'll change it after your PR.

@d00616 d00616 closed this Jan 4, 2015
@d00616 d00616 reopened this Jan 4, 2015
@LudwigKnuepfer
Copy link
Member

Here's my native review PR: d00616#1

@OlegHahm OlegHahm added the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Jan 6, 2015
* @return FLASH_ERROR_LOCKED
* @return FLASH_ERROR_TIMEOUT
*/
uint8_t flash_memcpy_fast(void *dest, const void *src, size_t n);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove this functionality. I have planned it to check alignment in test suite and for additional functionality. But while i implement an NRF51 version for flash i see that unaligned writes are impossible because i must write an word and wait for finishing opation to proceed with the next word.

@PeterKietzmann
Copy link
Member

Honestly I currently don't have an opinion here and won't find time to care about. Let's postpone for this release

@PeterKietzmann PeterKietzmann modified the milestones: Release 2016.07, Release 2016.04 Mar 23, 2016
@kYc0o
Copy link
Contributor

kYc0o commented Mar 23, 2016

I would vote for some file system instead of raw writings to the external memories (either flash or EEPROM). There is an effort in #2474 but it seems abandoned...

@OlegHahm
Copy link
Member

Having an interface to write into flash seems to be a prerequisite for using a filesystem on flash memory.

@kYc0o
Copy link
Contributor

kYc0o commented Mar 23, 2016

IMO we should use directly the low level driver depending on the memory type (I2C, SPI) which would be used by the file system

@OlegHahm
Copy link
Member

Hm, but using a filesystem might introduce some overhead when an application just wants to store some simple information onto persistent memory. Think of something like a persistent configuration for the radio (e.g. channel, PAN ID etc.).

@kYc0o
Copy link
Contributor

kYc0o commented Mar 23, 2016

Oh yes, that way it can be useful to easily write out of the filesystem memory space. I'll try to review this but as it's tagged for the next release I'll do it after this one :)

@LudwigKnuepfer
Copy link
Member

A flash abstraction layer is definitely desirable and can not be replaced by a file system.
One example: firmware update.

@kYc0o
Copy link
Contributor

kYc0o commented Mar 24, 2016

Yes I totally agree, but I would see it as a device driver that can be then leveraged by the file system (with simple functions like read/write/erase). Firmware updates can also leverage the FS to easily find the binaries needed for update.

* @return FLASH_ERROR_VERIFY
* @return FLASH_ERROR_ADDR_RANGE
*/
uint8_t flash_memcpy(void *dest, const void *src, size_t n);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

void *dest - the flash size may exceed the native address space.

@kYc0o
Copy link
Contributor

kYc0o commented Jul 22, 2016

Several other options exist now to add this new feature, so I'll postpone this one in order to give a detailed review of all the propositions.

@kYc0o kYc0o modified the milestones: Release 2016.10, Release 2016.07 Jul 22, 2016
@miri64
Copy link
Member

miri64 commented Sep 27, 2016

well the question is: do we want to keep this open now that there are some alternatives?

@OlegHahm
Copy link
Member

Maybe we should start a flash interface task force and consolidate the efforts?

@miri64
Copy link
Member

miri64 commented Oct 31, 2016

Postponed due to Feature Freeze.

@miri64 miri64 modified the milestones: Release 2017.01, Release 2016.10 Oct 31, 2016
@OlegHahm
Copy link
Member

@haukepetersen, how does this PR relates to the other efforts in this direction?

@haukepetersen
Copy link
Contributor

IMHO, this PR is 80-90% the same thing as the already merged 'periph/flashpage` interface - just by a magnitude more complex.. (e.g. for the nrf5x: 282 lines of code in this PR vs 47 lines for the flashpage driver). The main difference I see is that this PR allows to write parts of pages, or even multiple pages at once, while the existing interface allows only for writing one complete page at a time.

My suggestion would be to do the following:

  • add something like flashpage_write_raw(void *start, void *data, size_t len) to the existing flashpage interface
  • port the implementation for native to the flashapge interface

@kYc0o
Copy link
Contributor

kYc0o commented Jan 16, 2017

add something like flashpage_write_raw(void *start, void *data, size_t len) to the existing flashpage interface
port the implementation for native to the flashapge interface

+1 👍

@miri64
Copy link
Member

miri64 commented Jan 24, 2017

Closed as memo

@miri64 miri64 closed this Jan 24, 2017
@miri64 miri64 added the State: archived State: The PR has been archived for possible future re-adaptation label Jan 24, 2017
@jnohlgard jnohlgard added the Area: fs Area: File systems label Feb 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers Area: fs Area: File systems State: archived State: The PR has been archived for possible future re-adaptation Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants