335 automate flash file system hardware tests #366
Conversation
First step to test basic functionalities of our filesystem. Checks: - adding/moving/deleting files - blocking sectors for fpga - freeing blocked sectors
- move askEspForOkViaPico to AutomatedHWTests directory - change affected CMake files accordingly
Problem: - counter variable in "filesystemFindFittingStartSector" used to be uint8_t - during testing this led to unwanted behaviour caused by overflow Solution: . make counter uint32_t Additional: - introduced numberOfFreeSectors to filesystemConfig for easier testing - added matching behaviour to affecting functions - introduced early check for moving file into blocked sector
HerrCooker
force-pushed
the
335-automate-flash-file-system-hardware-tests
branch
from
84ea72c to
9522d32
Compare
|
@DavidFederl I don't understand why the formatting tests fail. Weren't these meant to just give warnings? |
They do fail, when the rules are violated. They just don't fix the problem anymore and leave that to the contributor. We discussed the we don't fail the workflow stuff for commitlint. But doesn't implement it yet. |
refactor(hal): apply clang format refactor(hal): apply clang format
HerrCooker
force-pushed
the
335-automate-flash-file-system-hardware-tests
branch
from
ccf2b21 to
afb8fb0
Compare
We could set the pipeline to not require the linters to pass before merging. I think i'd actually like that more than just raising a warning. |
That is very cool, |
|
I thought i was more specific in the commit message, by listing what this test is supposed to do. Not sure what i could add otherwise. Any ideas? |
| filesystemConfig->filesystemStartSector = 1019; | ||
| filesystemConfig->filesystemEndSector = 1023; | ||
| filesystemConfig->flash = flashConfig; | ||
| filesystemConfig->numberOfFreeSectors = 1019; | ||
|
|
There was a problem hiding this comment.
Where do these numbers come from btw? Can you find a way to make that clearer? E.g., using object like macros or enums?
There was a problem hiding this comment.
Overall this is quite a long function. Do you see reasonable ways to split it into smaller steps? Maybe at the points where comments are used for explanation atm?
| @@ -59,12 +58,11 @@ void filesystemInit(flashConfiguration_t *flashConfig, | |||
|
|
|||
| int32_t filesystemFindFittingStartSector(const filesystemConfiguration_t *filesystemConfig, | |||
| uint32_t numberOfRequiredBytes) { | |||
| int32_t newFileSector; | |||
| uint8_t counter = 0; | |||
| int32_t newFileSector = -1; | |||
There was a problem hiding this comment.
What is the semantics of -1 here? object like macro?
| filesystemConfig->fileSystem[filesystemConfig->numberOfEntries].entry.size = size; | ||
| filesystemConfig->fileSystem[filesystemConfig->numberOfEntries].entry.isConfig = isConfig; | ||
|
|
||
| // Set number of used sectors for this file | ||
| uint32_t numberOfUsedSectors = (size_t)ceilf( |
There was a problem hiding this comment.
not 100% sure rn, but these lines and the lines above seem to all handle a very specific aspect of the configuration. It might make sense to simplify the use here by wrapping them in functions. E.g. having
static inline void set_size(FsConfig *config, size_t size) {
config->fileSystem[config->numberOfEntries].entry.size = size;
}and afterwards just writing
set_size(filesystemConfig, size);makes the code a lot easier to digest.
| TEST_ASSERT_EQUAL(true, filesystemExists); | ||
| } | ||
|
|
||
| void testFileSystemDoesntExist() { |
There was a problem hiding this comment.
I might be wrong, but it seems, your test name is describing the test case, as in "given the case that the file system does not exist".
Often it is more helpful though, to describe the expected behaviour.
(Tbh, actually we'd have to do both, but unity out-of-the-box does not provide an easy way to do that 😢)
| TEST_ASSERT_EQUAL(false, filesystemExists); | ||
| } | ||
|
|
||
| void testFindFittingStartSector() { |
There was a problem hiding this comment.
That in turn is a really good name 👍
Implement first step for testing our filesystem. Future additions to these tests might be necessary.