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

FlashIAPBlockDevice init failing if using default constructor #9468

Closed
tommikas opened this Issue Jan 23, 2019 · 8 comments

Comments

Projects
None yet
5 participants
@tommikas
Copy link
Contributor

tommikas commented Jan 23, 2019

Description

Target: At least NRF52_DK
Toolchain: GCC_ARM
Build tools: mbed-cli
Mbed OS revision: 5.10.2 and newer

At least on NRF52_DK the default base address (defined here) causes init to fail.

Issue request type

[ ] Question
[ ] Enhancement
[x] Bug
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Jan 23, 2019

@dannybenor

This comment has been minimized.

Copy link

dannybenor commented Jan 23, 2019

@tommikas I do not see any component defined in targets.json. Are you adding FlashIAP in your mbed_app.json?

@davidsaada

This comment has been minimized.

Copy link
Contributor

davidsaada commented Jan 23, 2019

@tommikas FlashIAP block device is a special case where no default constructor can work for all boards. Reason is that flash addresses are different, block device can overrun the code and/or internal storage, and all in all much care should be taken when initializing this block device. Thus calling the default constructor without considering all the implications can be destructive - one can erase the code, the storage, configuration etc. Therefore an init failure is probably the best scenario. Maybe a better documentation or a better explained error can make things clearer, but all in all - this is the expected outcome. In addition, default constructor was kept for backward compatibility sake, as changing it broke few example codes.

@dannybenor

This comment has been minimized.

Copy link

dannybenor commented Jan 23, 2019

@tommikas why not using the KVStore static API that will provide an automatic address?

@tommikas

This comment has been minimized.

Copy link
Contributor Author

tommikas commented Jan 23, 2019

@tommikas I do not see any component defined in targets.json. Are you adding FlashIAP in your mbed_app.json?

Yep.

@tommikas FlashIAP block device is a special case where no default constructor can work for all boards. Reason is that flash addresses are different, block device can overrun the code and/or internal storage, and all in all much care should be taken when initializing this block device. Thus calling the default constructor without considering all the implications can be destructive - one can erase the code, the storage, configuration etc. Therefore an init failure is probably the best scenario. Maybe a better documentation or a better explained error can make things clearer, but all in all - this is the expected outcome. In addition, default constructor was kept for backward compatibility sake, as changing it broke few example codes.

I'm fine with all that, as long as it is indeed documented clearly and the failure is explicit. Fixing it now that I know what the problem is is trivial, but figuring out what exactly was wrong took too much work.

In addition, default constructor was kept for backward compatibility sake, as changing it broke few example codes.

As a user I'd prefer it breaking at compile time rather than run time. I understand not wanting to break the API, but that doesn't help much if the functionality breaks.

@tommikas why not using the KVStore static API that will provide an automatic address?

Defining the flash address and size statically for the targets I want to support isn't a problem.

@ciarmcom

This comment has been minimized.

Copy link
Member

ciarmcom commented Jan 23, 2019

@dannybenor

This comment has been minimized.

Copy link

dannybenor commented Jan 23, 2019

@tommikas thanks for pointing this. We will fix it by removing the default values from the json file. This will cause compilation error if you do not provide addresses to the constructor or add them to the json file

@davidsaada

This comment has been minimized.

Copy link
Contributor

davidsaada commented Jan 23, 2019

PR created to resolve this issue. Note that it can't be resolved in compilation time, as the compiler can't tell whether the default constructor is used or not. Added a run time assertion that points out the error - better than before, where it just didn't work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.