Skip to content

NVStore.cpp compiler warning removal (os_ret) #10485

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 3, 2019
Merged

NVStore.cpp compiler warning removal (os_ret) #10485

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions features/storage/nvstore/source/nvstore.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -811,7 +811,6 @@ int NVStore::init()
uint32_t free_space_offset_of_area[NVSTORE_NUM_AREAS];
uint32_t init_attempts_val;
uint32_t next_offset;
int os_ret;
int ret = NVSTORE_SUCCESS;
int valid;
uint16_t key;
Expand Down Expand Up @@ -869,8 +868,8 @@ int NVStore::init()

// Find start of empty space at the end of the area. This serves for both
// knowing whether the area is empty and for the record traversal at the end.
os_ret = calc_empty_space(area, free_space_offset_of_area[area]);
MBED_ASSERT(!os_ret);
ret = calc_empty_space(area, free_space_offset_of_area[area]);
MBED_ASSERT(!ret);

if (!free_space_offset_of_area[area]) {
area_state[area] = NVSTORE_AREA_STATE_EMPTY;
Expand All @@ -891,8 +890,8 @@ int NVStore::init()

// We have a non valid master record, in a non-empty area. Just erase the area.
if ((!valid) || (key != master_record_key)) {
os_ret = flash_erase_area(area);
MBED_ASSERT(!os_ret);
ret = flash_erase_area(area);
MBED_ASSERT(!ret);
Copy link
Contributor

Choose a reason for hiding this comment

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

It is slightly worrying that the errors due to HW problems are detected only on debug-builds. If the remaining code survives from storage error, what is the point of assert here?

Copy link
Contributor

@kjbracey kjbracey Apr 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Do we expect hardware problems to be indicated by error returns from flash functions though?

It's quite possible that the only error returns would be for parameter errors like address being out of range, and we only find out flash failure by actually trying to use it and finding it doesn't hold data correctly.

But I would agree that if these functions can indicate hardware problems, there should be paths to deal with that. Asserts should be reserved for catching should-be-impossible design errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tend to agree here. :-) But, well out of scope for this PR in a way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tend to agree here. :-) But, well out of scope for this PR in a way.

Please create a tracking issue for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, IOTSTOR-832

Copy link
Contributor

Choose a reason for hiding this comment

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

@kjbracey-arm at least some drivers seem to notify HW/driver problems, not just user errors:

mbed-os/drivers/FlashIAP.cpp

Line 204 in cc94690

ret = flash_erase_sector(&_flash, addr);

->

mbed-os/targets/TARGET_Cypress/TARGET_PSOC6/flash_api.c

Lines 41 to 50 in d2e9fde

int32_t flash_erase_sector(flash_t *obj, uint32_t address)
{
(void)(obj);
int32_t status = 0;
if (Cy_Flash_EraseRow(address) != CY_FLASH_DRV_SUCCESS) {
status = -1;
}
return status;
}

area_state[area] = NVSTORE_AREA_STATE_EMPTY;
continue;
}
Expand Down Expand Up @@ -939,8 +938,8 @@ int NVStore::init()
_active_area = 1;
}
_active_area_version = versions[_active_area];
os_ret = flash_erase_area(1 - _active_area);
MBED_ASSERT(!os_ret);
ret = flash_erase_area(1 - _active_area);
MBED_ASSERT(!ret);
}

// Traverse area until reaching the empty space at the end or until reaching a faulty record
Expand Down