-
Notifications
You must be signed in to change notification settings - Fork 3k
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
STORAGE: Merging feature-storage branch commits to master #3762
Conversation
@geky Please could you check this over to make sure I haven't missed any commit from the feature-storage branch? Thanks. |
This looks good to me 👍 Thanks for going through the pain of rebasing this branch. As a side note, here are the remaining important paches: #3766, #3764, #3747. They can just as easily be moved against master once this first push is in, so I don't think we need to worry about them right now. Is there any other work that needs to be done on this pr? |
Hi I've been reviewing the SPIFBlockDevice and the Microchip flash documentation and thought I should provide some First some definitions with respect to flash memories:
For the above part a simple block API would probably have:
<<<<<<< TEXT/FILES DELETED See email for details. I've attached to this email the following files from TEXT DELETED >>>>>>>>>>> Note the following:
By way of comparison, here is the yaffs block API (nand): int yflash2_EraseBlockInNAND(struct yaffs_dev *dev, int blockNumber); and interesting defines: /* Constants for YAFFS1 mode / Observations for the yaffs API:
It will be interesting to compare the other APIs to those above. So what does this mean for our current BlockDevice API?
|
platform/retarget.h
Outdated
#ifndef RETARGET_H | ||
#define RETARGET_H | ||
|
||
#if defined TOOLCHAIN_ARM_STD || defined TOOLCHAIN_IAR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- why this file is named retarget ? I wonder if more specific name would not be better - It is mainly about errno.h definitions, not something like mbed_errno/errno_codes or any suggestions.
- why it does not include errno.h if it redefines some symbols?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I currently has errno redefinitions and stat definitions, but it could be a resonable place for other redefitions. I think retarget.h is reasonable unless we want many small files for specific purposes (should be break apart retarget.cpp as well in that case?).
You're right, retarget.h should probably include errno.h 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, agreed with gatekeepers that retarget.h is acceptable name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment reads about ARMC5 differences but the inclusion is for IAR and ARMC5. Needs some clarification and use compiler emitted macros.
And just so I understand, the intent is to unify the errno values to match POSIX definitions across the 3 toolchains?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the intent is to unify errno values across toolchains, and supply values where missing. I've updated comments and changed to use compiler emitted macros.
features/filesystem/fat/FATMisc.h
Outdated
|
||
#include "ff.h" | ||
|
||
void fat_filesystem_set_errno(FRESULT res); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing docs for this function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
features/filesystem/fat/FATMisc.h
Outdated
@@ -0,0 +1,27 @@ | |||
/* | |||
* mbed Microcontroller Library | |||
* Copyright (c) 2006-2016 ARM Limited |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
be aware of years, this is a new file - 2017
features/filesystem/fat/FATMisc.cpp
Outdated
#include "platform/retarget.h" | ||
|
||
|
||
/* @brief Set errno based on the error code returned from underlying filesystem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Place this in the declaration header file as I commented below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
|
||
// FAT driver functions | ||
DWORD get_fattime(void) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please fix formatting {
new line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Astyle tool run on FATXxx files to correct multiple instances of this formatting problem.
@@ -35,13 +36,46 @@ using namespace mbed; | |||
*/ | |||
class FATFileSystem : public FileSystemLike { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This header file could contain informatin about thread safety as our API do (I noticed it is as there are lock/unlock methods)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation added.
* | ||
* @return Size of a eraseable block in bytes | ||
*/ | ||
virtual bd_size_t get_erase_size(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be declared with const as they are getters (read-only) ? This would apply to all get methods in this patch if yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They could be, I originally did not since it may require implementors learn to use mutable for cases where non-const SPI transaction may occur.
If we're pushing for const (and more adoption of const+mutable where simple hardware transaction are required) I don't see a reason to not slap const on these guys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_xxx_size() const added for get_write_size(), get_read_size(), get_program_size(), get_erase_size()
* stack tracking statistics are enabled. If this is the case, build dummy | ||
* tests. | ||
*/ | ||
#if ! defined(TOOLCHAIN_IAR) && ! defined(TARGET_KL25Z) && ! defined(MBED_STACK_STATS_ENABLED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just KL25Z specific or because target does not have enough memory (this might lead to skip this test for many targets, the list will grow here) ?
I cant spot here a reason why it fails to build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@0xc0170 This only skips the test for IAR building for KL25Z target with MBED_STACK_STACK_ENABLED enabled as the binary is too big. One of the CI builds some mbed_micro jobs and this fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bridadan may want to have a word.
So far we have simply been splitting up tests when they get too big. Not sure if that is appropriate here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the past we have split up tests due to their size, so I'd say this is an acceptable solution if there is a reasonable way to split this up.
A test definitely should not change its behavior due to the target or the toolchain. So the way it is currently implemented should change.
Could we not simply reduce the size of the HeapBlockDevice
? Does it have to be 16*BLOCK_SIZE
? That's fairly large (8k if I'm doing my math right). The KL25Z only has 16K of RAM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My proposal for this would be: would be good to split now because obviously our tools do not support this use case. But is that a long term solution ? What size requirement should be set for a test? Or is there any other way to handle this?
If there's a test that does not fit all targets (we will have definitely basic tests that should exercise API and fit to any target) and then tests that do lot of things. Should our tools handle this as not supported (not a failure, a target cant support this test thus wont be reported as failure), but will be shown in the result. Thus a user would know x tests were not run due to a size failure (not enough RAM or FLASH).
Might be worth creating an issue for this to continue discussion there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most tests should mimic real-world use cases. If a target can't fit a real-world use case of a feature or API in it's FLASH or RAM, then it sounds like that target shouldn't support that feature! Or the API needs to do some optimizations!
If the feature is mission critical for mbed OS, then I would suggest the following options:
- Move the API and tests behind a FEATURE_ directory and enable it for all relevant platforms.
- Remove the target from the supported list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe none of the current tests coming in need to be split. The large filesystem tests now live over here:
https://github.com/ARMmbed/sd-driver/tree/master/features/TESTS/filesystem
We should be able to just remove this compile time conditions
Just some addition to the the message above about sector size vs page size, that might help. We had similar problem in flash api - we dont want to mix these two worlds, the message above just reminded me some discussions we had with @c1728p9, thus I'll add some notes here that could help: A sector size might not be the same as a page size (I use this terminology as it comes from flash API). Thus erase() operates on different boundaries than program_page(). It might not be ideal to provide write() that does operate on two different things, explanation below.
Be aware that sector size might be big, and might not fit in RAM. Therefore it might be better to provide erase() and program_page vs only write_sector(). Lets say there's a device with 128kB sector only. write() method would fail - a user can't allocate the buffer required for this operation. But it would be fine if they do it in small chunks (multiple pages after sector_erase()) - flow would be erase_sector(), program_page() * multiple times to be able to fill an entire sector or just partially. I already highlighted a need for get_sector_size with an address (sectors might differ based on the address) for devices with various sector sizes. cc @c1728p9 |
This post summarises my requested changes. Full explanation in the attached file block_api_notes_for_chris_20170221_for_pr3762.txt, and email. I think the changes should be done after 5.4 as a patch release so as not to delay this PR.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few nits here and there but in general nice one.
* stack tracking statistics are enabled. If this is the case, build dummy | ||
* tests. | ||
*/ | ||
#if ! defined(TOOLCHAIN_IAR) && ! defined(TARGET_KL25Z) && ! defined(MBED_STACK_STATS_ENABLED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TOOLCHAIN_XXX should be replaced by compiler emitted macros.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simonqhughes, actually this check should be removed
return BD_ERROR_PARAMETER; | ||
} | ||
|
||
#ifndef NDEBUG |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this left over instrumentation during development?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. The heap block device is primarily used for testing (and backs the inbound storage tests).
When in debug mode, erased blocks are clobbered to help catch errors with unintentionally erasing the wrong blocks. Otherwise, erase is a nop.
} | ||
|
||
/* ARMCC doesnt support stat(), and these symbols are not defined by the toolchain. */ | ||
#ifdef TOOLCHAIN_GCC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use compiler emitted macro rather than labels
platform/retarget.cpp
Outdated
st->st_mode = S_IFCHR; | ||
return 0; | ||
} | ||
|
||
/* todo: 20170120 this should now be made to work as _stat() is implemented */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this todo completed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No this todo is still valid, fstat != stat
platform/retarget.h
Outdated
#ifndef RETARGET_H | ||
#define RETARGET_H | ||
|
||
#if defined TOOLCHAIN_ARM_STD || defined TOOLCHAIN_IAR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment reads about ARMC5 differences but the inclusion is for IAR and ARMC5. Needs some clarification and use compiler emitted macros.
And just so I understand, the intent is to unify the errno values to match POSIX definitions across the 3 toolchains?
tools/paths.py
Outdated
@@ -24,7 +24,9 @@ | |||
BUILD_DIR = getenv("MBED_BUILD_DIR") or BUILD_DIR | |||
|
|||
# Embedded Libraries Sources | |||
LIB_DIR = join(ROOT, "features/unsupported") | |||
FEATURES_DIR = join(ROOT, "features") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like mbed 2 specific testing was removed so this should be "features/unsupported" as it was
One more set of updates needed:
|
Since only failure and success will be returned, lets get rid of the error code type all together for now and use an integer like the other APIs. |
👍 👍 Sorry for not responding in detail to concerns, currently focused on getting this branch merged. @c1728p9, @sg- I should have pushed a commit that adds the last minute changes from our offline discussion (related to block device api):
@simonqhughes, sorry for pushing to your branch, you should be able to merge into any local copies you have with git pull. Very random comment:
Correct me if I'm wrong, but this can be introduced with an overload in a backwards compatible manner |
tools/tests.py
Outdated
@@ -853,7 +817,7 @@ | |||
{ | |||
"id": "RTOS_9", "description": "SD File write-read", | |||
"source_dir": join(TEST_DIR, "rtos", "mbed", "file"), | |||
"dependencies": [MBED_LIBRARIES, RTOS_LIBRARIES, TEST_MBED_LIB, FS_LIBRARY], | |||
"dependencies": [MBED_LIBRARIES, RTOS_LIBRARIES, TEST_MBED_LIB], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this test functional without fs library dependency? should not be removed? same for 2 more test below
FS_PATH = join(LIB_DIR, "fs") | ||
FAT_FS = join(FS_PATH, "fat") | ||
SD_FS = join(FS_PATH, "sd") | ||
FS_LIBRARY = join(BUILD_DIR, "fat") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sg- Just to confirm, fs library moves out of unsupported folder, gets all this updates, thus these 4 lines shall be removed and any dependencies on these (there are some mbed 2 tests that shall be removed then).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simonqhughes Please look at jenkins CI failure. From what I found, there's at least one |
pr_head shows this:
|
@simonqhughes thanks for fixing the changes I missed 👍 |
Result: FAILUREYour command has finished executing! Here's what you wrote!
|
Public test result server was full again, restarting. /morph test-nightly |
@geky @simonqhughes looks like we have merge conflicts |
Result: FAILUREYour command has finished executing! Here's what you wrote!
OutputTest failed! |
Looks like some shuffling around of the block devices revealed an invalid write to unallocated memory. The write was only for debugging purposes so it has been removed. |
Hmmmm
@bridadan, if you get a chance, could you be a dear and restart the uvisor CI? I'd rather not distract morph test by force pushing. |
retest uvisor |
Retest uvisor |
/morph test-nightly |
retest uvisor |
1 similar comment
retest uvisor |
Result: FAILUREYour command has finished executing! Here's what you wrote!
OutputTest failed! |
the latest results: seems like few platforms fail with |
…s an SDCard dependent test.
/morph test-nightly |
pr_head failing with: [wlan-8022] | interface_up_down | fail | dut1 cmd: 'ifconfig wlan0 --ap | | K64F | 40.438019 | @marhil01 @SeppoTakalo these look unrelated to the actual PR under test ?? Potentially blocking 5.4 release...Is there a device failure? |
I've kicked off a rebuild on pr-head to see if that fixes the issue |
/morph test-nightly |
Result: SUCCESSYour command has finished executing! Here's what you wrote!
OutputAll builds and test passed! |
|
Hi after merge of this PR, the ci-test-shield tests for SPI FIle System can't build anymore as the sympbols are multiple defined. When resolving the duplication locally and removing the file system that was used primarily in ci-test-shield, then the test-api-spi tests that were passing before are now FAILED. Maybe I missed something ... so has this been tested or is there any plan to also update ci-test-shield repo following this PR update ? |
@geky can you take a note to update the test here https://github.com/ARMmbed/ci-test-shield |
Can do. There is additional work to support the filesystem tests there as well. |
@LMESTM This was expected, we will update the tests (the current supported mbed lib there is older). It will be updated with fixes. Feel free to capture this into a tracking issue to ci test shield repo.
OK |
Description
This PR is to bring the new commits added to the feature-storage branch into master in preparation for creating the mbed-os 5.4 release candidate. The time line is as follows:
In addition to the commits cherry-picked from the feature-storage branch, the following modifications have been made to make a clean delivery to master:
The files that have been removed are as follows:
As the above files are no longer in the final PR (but many commits are present including changes to those files before being removed), any changes to the above files in intermediate commits can be ignored.
Background Information
It appears that (for reasons unknown) rebasing of the feature-storage branch with master (so that PAL team could have reasonably up-to-date feature-storage branch) has created commits with new sha's, now making it difficult to rebase the feature-storage branch with master again, or merge the existing feature-storage branch into master. After discussion with Martin, it was decided the best course of action was to:
The list of commits cherry-picked into this branch is in the attached file.
Status
undergoing CI testing
feature_storage_commits.txt