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

Allow to redefine nRF's PSTORAGE_NUM_OF_PAGES outside of the mbed-os #3836

Merged
merged 1 commit into from Mar 6, 2017

Conversation

Projects
None yet
5 participants
@crontab
Contributor

crontab commented Feb 24, 2017

Description

By default the number of pstorage pages is set 1 and all addresses are calculated in the pstorage module accordingly. Nordic recommends changing this macro to whatever number is suitable for the app (see this answer from Petter Myhre) which is not quite elegant given that pstorage_platform.h is part of the mbed-os repo. With this modification you can e.g. define PSTORAGE_NUM_OF_PAGES on the command line, however note that you should rebuild mbed-os with this setting as it affects pstorage_platform.c.

This patch modifies the source for both nRF51 and nRF52 families.

Allow to redefine nRF's PSTORAGE_NUM_OF_PAGES outside of the mbed-os …
…source

By default the number of pstorage pages is set 1 and all addresses are
calculated in the pstorage module accordingly. Nordic recommends
changing this macro to whatever number is suitable for the app (see
https://devzone.nordicsemi.com/question/53066/what-will-be-the-starting-
address-of-pstorage-page-how-we-can-change-it/?answer=53085#post-id-5308
5) which is not quite elegant given that pstorage_platform.h is part of
the mbed-os repo. With this modification you can e.g. define
PSTORAGE_NUM_OF_PAGES on the command line, however note that you should
rebuild mbed-os with this setting as it affects pstorage_platform.c.
@pan-

This comment has been minimized.

Member

pan- commented Feb 24, 2017

CC @nvlsianpu

@crontab Thanks for your submission, it is a good change but I would like some form of documentation explaining what define have to be provided to override the default number of pages used by the pstorage module.

@crontab

This comment has been minimized.

Contributor

crontab commented Feb 24, 2017

@pan- could you point out which part of the documentation should be amended? Note that the pstorage module comes from Nordic's own SDK, and I honestly don't know how mbed handles such imports of 3rd party code.

@nvlsianpu

This comment has been minimized.

Contributor

nvlsianpu commented Feb 24, 2017

@crontab How you want to inject PSTORAGE_NUM_OF_PAGES into build?

@crontab

This comment has been minimized.

Contributor

crontab commented Feb 24, 2017

@nvlsianpu @pan-

There is no need to inject if you don't want to change the default, which is exactly one page of Flash memory available to the app. However, if you do want to change it, you need to pass -DPSTORAGE_NUM_OF_PAGES=<N> to the compiler. As I understand it mbed CLI doesn't support this directly, so it would be e.g. via the generated Makefile, or the IDE you're using for building your project.

@nvlsianpu

This comment has been minimized.

Contributor

nvlsianpu commented Feb 24, 2017

mbed-cli supports -D....

This changes looks good for me. It won't changes target behaviour until -DPSTORAGE_NUM_OF_PAGES (or equivalent) will be used. I accept this PR from Nordic side.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 27, 2017

@crontab would be worth sending this upstream (to their SDK for consideration), as this might get overwritten in the next SDK update. Plus there are lot of config options that could be set, and having this option to change them without touching the source might be worthwhile

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 27, 2017

/morph test

@0xc0170 0xc0170 added the needs: CI label Feb 27, 2017

@crontab

This comment has been minimized.

Contributor

crontab commented Feb 27, 2017

@0xc0170 thanks! Sounds reasonable to let Nordic know and prevent future overrides, but I have no idea what the established procedure is.

@mbed-bot

This comment has been minimized.

mbed-bot commented Feb 27, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1599

All builds and test passed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 6, 2017

@0xc0170 thanks! Sounds reasonable to let Nordic know and prevent future overrides, but I have no idea what the established procedure is.

@nvlsianpu can help you.

@0xc0170 0xc0170 merged commit 1be7418 into ARMmbed:master Mar 6, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has started
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment