Settings Driver #775

lluwang · 2016-10-08T14:50:00Z

This is one of PRs for 9.2.8.

nibanks

You need some changes fo memory allocations.

nibanks · 2016-10-08T19:53:02Z

src/core/common/settings.cpp

+
+void reorderSettingsBlock(void)
+{
+    uint8_t *ramBuffer = (uint8_t *)malloc(OPENTHREAD_CONFIG_SETTINGS_PAGE_SIZE);


You can't call malloc/free directly. When this runs on Windows in kernel mode, you have to call different functions to allocate memory.

codecov-io · 2016-10-09T01:53:12Z

Current coverage is 73.07% (diff: 86.76%)

Merging #775 into master will increase coverage by 1.04%

@@             master       #775   diff @@
==========================================
  Files           105        108     +3   
  Lines         15899      16247   +348   
  Methods        2164       2208    +44   
  Messages          0          0          
  Branches       1888       1950    +62   
==========================================
+ Hits          11452      11873   +421   
+ Misses         3833       3736    -97   
- Partials        614        638    +24

Powered by Codecov. Last update 10f9204...f66018b

jwhui · 2016-10-09T01:57:03Z

src/core/common/settings.cpp

+
+void reorderSettingsBlock(void)
+{
+    uint8_t ramBuffer[OPENTHREAD_CONFIG_SETTINGS_PAGE_SIZE];


Buffering settings in RAM to rewrite the settings block does allow this to operate on a single flash page. However, there are a couple issues with this:

If the device resets (or loses the memory) for any reason during this operation, all the settings will be lost.

This requires buffering a significant amount of state in memory before writing it back.

I'd prefer a solution that swaps between two different flash pages. Using two flash pages would avoid needing to buffer all the settings in RAM and can be made resilient to device reboots.

Thoughts?

Agree. It is better.

I will use one flash page as the swap page (similar to the RAM space).
The possible problem is that this page may be read/write many more times than others.

Using two flash pages should not result in more flash writes. Writes happen on a single flash page until there is no more space. When space runs out on page A, the implementation can pull out the useful data, write it to page B, then use page B until that runs out of space. Then switch back to page A.

Yes. But may it need more than one flash page to store the settings?

Another way is to always use 2* N flash pages. N 's default is 1, but can be larger like 2, 3....

Yes, the simplest is to support at least 2x the size needed for storing settings. The number of pages should be computed based on an OpenThread compile-time configuration and a platform-specific page size. When I mentioned "page" above, I really meant "flash space used for settings".

nibanks · 2016-10-09T03:32:00Z

I've got a more basic question of why OpenThread is dealing with something so platform specific as flash pages to begin with. On Windows this makes no sense. If we need to persist a setting, we'd likely just write it to a registry key. Shouldn't the otPlat* API be something basic like read/write setting? We can move this code for converting settings to flash pages into an intermediate library a platform can optionally use.

jwhui · 2016-10-09T03:45:49Z

The otPlatSettings* API is the only real requirement for OpenThread. At the same time, we expect a common target for OpenThread are MCUs with on-chip flash storage capability. For that reason, we've decided to also define a common implementation of otPlatSettings* that lives on top of otPlatFlash* APIs. The intent is that this implementation of otPlatSettings* is only provided for convenience and can be replaced on a given platform. I agree that this PR in its current form does not completely fulfill the intent.

lluwang · 2016-10-09T03:51:12Z

@jwhui , so would you prefer put settings driver in example/platform/posix and example/platform/cc2538?

jwhui · 2016-10-09T04:07:00Z

@luwannest, we can leave it where it is now. However, we should provide compile-time options to allow building with an alternative platform-specific implementation of otPlatSettings*. I wouldn't necessarily worry about those compile-time options in this PR, we can introduce those in a separate PR.

jwhui · 2016-10-11T19:46:28Z

@lluwang, thanks for updating this PR. I'm a little concerned about staging the changes in RAM. I think we could simplify this by removing support for:

otPlatSettingsBeginChange()
otPlatSettingsCommitChange()
otPlatSettingsAbandonChange()

The above functions are noted as optional in the docs.

Also, we need to update the API to support multiple instances as we have done with the rest of the OpenThread APIs. At the same time, we need to avoid using globals within the implementation.

jwhui · 2016-10-11T19:46:54Z

src/core/common/settings.cpp

+{
+    kMaxStageAddNum = 3,
+    kMaxStageDeleteNum = 3,
+    kMaxStageDataLen = 32,


Not large enough to support Operational Datasets.

I have removed otPlatSettingsChangesBegin/otPlatSettingsCommitChanges/otPlatSettingsAbandonChanges.

jwhui · 2016-10-11T19:47:13Z

src/core/common/settings.cpp

+static uint8_t sStageAddSettingsNum;
+static uint16_t sStageAddSettingsBufLength;
+static struct stageDeleteSettingsBlock sStageDeleteSettingsBlock[kMaxStageDeleteNum];
+static uint8_t sStageDeleteSettingsNum;


Avoid using globals.

I still keep using sBase and sUsedSize as global variables,and failed to find a way not using them as global...

jwhui · 2016-10-11T19:47:52Z

include/platform/settings.h

@@ -229,7 +229,7 @@ ThreadError otPlatSettingsAdd(uint16_t aKey, const uint8_t *aValue, int aValueLe
 *  @retval kThreadError_NotImplemented
 *          This function is not implemented on this platform.
 */
-ThreadError otPlatSettingsDelete(uint16_t aKey, int index);
+ThreadError otPlatSettingsDelete(uint16_t aKey, int aIndex);


Need to update the otPlatSettings* APIs to take an otInstance.

otInstance has been added for otPlatSettings_.
I will submit a separate PR for otPlatFlash_.

jwhui · 2016-10-11T19:49:40Z

src/core/common/settings.cpp

+                         error = kThreadError_NoBufs);
+        }
+
+        otPlatFlashWrite(sBase + sUsedSize, reinterpret_cast<uint8_t *>(stageBlock),


We need to make sure the write is atomic. For example, what if the node reboots in the middle of this flash write? What probably needs to happen here is first write out the block header and data. Then when that's complete, modify a bit in the header to indicate that the write has completed.

I split kSettingsAdd flag into kBlockAddBeginFlag and kBlockAddCompleteFlag for atomic write.

jwhui · 2016-10-11T19:52:39Z

src/core/common/settings.cpp

+
+        if ((!(block.flag & kBlockAddFlag) && block.flag & kBlockDeleteFlag) && (block.key == aKey))
+        {
+            SuccessOrExit(error = otPlatSettingsDelete(aKey, -1));


Should not delete the old setting until the new setting is written. Maybe a simpler approach here is simply not to delete. A later block with the same key and index 0 indicates that all prior blocks are invalidated.

I have changed the logic here.
otPlatSettingsSet is otPlatSettingsAdd with index 0.

The block entry is valid only when

kSettingsAddComplete flag is not set

kSettingsDelete flag is set

there is not block entry containing the same key and <= index after this block entry

- remove support for otPlatSettingsChangesBegin/otPlatSettingsCommitChanges/otPlatSettingsAbandonChanges for embeded system - one block entry is valid, when addComplete not set and Delete is set and not smaller index with the same key found after this block entry - add otInstance support

jwhui · 2016-10-12T06:08:33Z

src/core/common/settings.cpp

+} OT_TOOL_PACKED_END;
+
+static uint32_t sBase;
+static uint32_t sUsedSize;


You can put these in the otInstance structure and reference these based on the otInstance pointer passed in.

jwhui · 2016-10-12T06:14:35Z

src/core/common/settings.cpp

+        otPlatFlashRead(address, reinterpret_cast<uint8_t *>(&block), sizeof(block));
+
+        if (!(block.flag & kBlockAddCompleteFlag) && (block.flag & kBlockDeleteFlag) &&
+            (block.key == aKey) && (block.index == aIndex || aIndex == -1))


Consider we have (A, B, C) and delete B (i.e. index 1), the resulting set will be (A, C) where index of A is 0 and C is 1. The current implementation here does not properly update the index.

I don't actually think you need to store the index. You only need to store the first item (i.e. index 0). That way, if I call delete on index 1 twice, both B and C will be deleted appropriately. If we avoid storing the actual index, I think that will simplify the implementation.

jwhui · 2016-10-12T06:23:40Z

src/core/common/settings.cpp

+enum
+{
+    kSettingsFlagSize = 4,
+    kSettingsBlockDataSize = 32,


You will need to buffer up to 255 bytes to support the Operational Datasets.

- Move global variable to otInstance

nibanks · 2016-10-12T13:27:03Z

tests/unit/Makefile.am

+test_settings_LDADD          = $(COMMON_LDADD)  \
+                               $(top_builddir)/examples/platforms/posix/libopenthread-posix.a \
+                               $(NULL)
+test_settings_SOURCES        = test_settings.cpp


Can we please follow the model of the tests not depending on a specific platform? Can you please include the test_platform.cpp as the other tests above do?

In this case, the implementation in settings.cpp depends on a working implementation of otPlatFlash*. We could include examples/platforms/posix/flash.c directly, but that would still mean this test depends on a specific platform. Thoughts?

The issue here is that to include it in the Windows unit test project it needs to not depend on the platform library. What if we created an in memory implementation of the functions implemented in flash.c, and put those in test_platform.cpp? Is the goal here to test the platform code or the core code?

I think a "mock" in-memory implementation of otPlatFlash* is reasonable. @lluwang, thoughts?

But till my understanding, it is almost the same as what posix does except that using in-memory space but not posix "flash"?

The idea is to remove any platform-specific dependencies (i.e. file operations).

lluwang · 2016-10-13T05:09:44Z

Or is it a bad way to allocate this big (256Kbytes) buffer?

jwhui · 2016-10-13T05:24:31Z

@lluwang, I just tried running make -f examples/Makefile-posix distcheck on OSX and I see the same failure as on travis.

lluwang · 2016-10-13T07:27:10Z

Thanks @jwhui !

nibanks · 2016-10-13T13:19:50Z

tests/unit/test_settings.cpp

+
+void RunSettingsTests(void)
+{
+    TestSettingsInit();


Can you add these tests to test_windows.cpp?

nibanks · 2016-10-14T13:51:24Z

To get appveyor building, you need to fix the warning in settings.cpp as shown here.

nibanks · 2016-10-14T13:54:29Z

Actually, I think you can just not include settings.cpp in libopenthread_k for now, since the kernel mode version of the library is what I use in the driver on Windows, and I will implement my own versions of the otPlat* APIs. But it would be great still to fix those warnings.

lluwang · 2016-10-14T15:24:53Z

Could I know how test_windows.cpp run the unit test?
Will it run TestSettingsInit, TestSettingsAdd, TestSettingsDelete, TestSettingsSet, TestSettingsSwap in sequence?

nibanks · 2016-10-14T15:26:59Z

It looks like it is running your unit tests, and one is failing. Assertion failed at:

at TestSettingsSwap() in c:\projects\openthread\tests\unit\test_settings.cpp:line 139

nibanks · 2016-10-14T15:31:30Z

It looks like it doesn't necessarily run the tests in sequence. If the tests have dependencies among each other, they aren't really separate tests then. Either you need to make them independent, or make Windows just run RunSettingsTests and not each individual test.

jwhui · 2016-10-14T19:51:39Z

From Seth Rickard s-rickard@ti.com:

The area your flash driver is writing begins at 0x00239000 and continues for a few pages. This puts the NV storage area about 228K into the chip’s flash. This is well above the code that is currently in OpenThread, but the area is not reserved in the linker script. It would be possible to write an application using OpenThread that has code in that area, without the linker complaining. The application would end up being accidentally overwritten by the settings.

The linker script might look like
SECTIONS
{
    /* place non-volatile storage in pages 254 and 255 */
    .nv_storage 0x7F800:
    {
        _nv_begin = .;
        . += 0x800;
        _nv_page_one = .;
        . += 0x800;
        _nv_end = .;
    } > FLASH
}
Then in the c file, finding the addresses would look like
extern char _nv_begin;
extern char _nv_page_one;
extern char _nv_end;

int someFunc()
{
    char *flash_base = &_nv_begin;
    uint32_t size_of_page = (uint32_t)(&_nv_page_one - &_nv_begin);
    uint8_t num_of_pages = (uint8_t)((&_nv_end - &_nv_begin) / size_of_page);
}
This way you don’t have to use #define s for the base address and page size

lluwang · 2016-10-17T01:23:13Z

If we do like this, is it specific to some platforms? Though settings driver may always only be used in embedded platform.

For the potential flash error, do we need to have a component to manage the flash layout?

nibanks

Looks good.

nibanks · 2016-10-17T02:08:58Z

etc/visual-studio/libopenthread_k.vcxproj

@@ -23,6 +23,7 @@
    <ClCompile Include="..\..\src\core\coap\coap_server.cpp" />
    <ClCompile Include="..\..\src\core\common\logging.cpp" />
    <ClCompile Include="..\..\src\core\common\message.cpp" />
+    <ClCompile Include="..\..\src\core\common\settings.cpp" />


Don't add this file to the kernel mode version.

jwhui · 2016-10-17T04:53:44Z

LGTM 👍 , we can address Seth's concerns in a separate PR.

…ub_aaf5a57 to master * commit '4da26717b402603ae58dd17c6cad8c33c2a34753': Cli: update 'keysequence' to make keyswithguardtime configurable (openthread#815) Settings Driver (openthread#775) ncp: Added sniffer host tool. (openthread#773)

…a` (openthread#775) Bumps [third_party/openthread/repo](https://github.com/openthread/openthread) from `5bc74b7` to `fa09e8a`. - [Commits](openthread/openthread@5bc74b7...fa09e8a) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

nibanks suggested changes Oct 8, 2016

View reviewed changes

jwhui requested changes Oct 9, 2016

View reviewed changes

jwhui added enhancement P1 labels Oct 9, 2016

jwhui added this to the 0.01.00 (Initial Official Release) milestone Oct 9, 2016

jwhui self-assigned this Oct 9, 2016

lluwang force-pushed the pr/settings_driver branch from 757819c to 9528816 Compare October 10, 2016 06:55

Lu Wang added 6 commits October 10, 2016 17:10

Settings Driver

61ee3a0

Use OpenThread Authors

33d13af

Use swap flash page

7455ea8

Fix for flash driver

7c86e34

Add settings posix unit test

a2d058c

Make pretty

144476f

lluwang force-pushed the pr/settings_driver branch from 9528816 to 144476f Compare October 10, 2016 09:14

Remove tmp folder generated by setting's unit test

1a62937

jwhui requested changes Oct 11, 2016

View reviewed changes

jwhui reviewed Oct 12, 2016

View reviewed changes

- Remove index field

7d961ed

- Move global variable to otInstance

nibanks reviewed Oct 12, 2016

View reviewed changes

Lu Wang added 2 commits October 13, 2016 10:19

Remove platform-specific dependency while running settings unit test

7fc5ab4

Init flash buffer

2748f3c

Fix bug in settings unit test

270bd94

nibanks reviewed Oct 13, 2016

View reviewed changes

tests/unit/test_settings.cpp

void RunSettingsTests(void)

{

TestSettingsInit();

Copy link

Contributor

nibanks Oct 13, 2016

Choose a reason for hiding this comment

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

Can you add these tests to test_windows.cpp?

Lu Wang added 6 commits October 14, 2016 06:15

Add seting unit test tests to test_windows

136705c

Fix check error

7631d6e

Include test settings source code for window

1704568

Remove header from settings unit test that does not support in windows

e0e37c9

Remove header in settings uint test that does not support in windows

32d5811

Add settings driver to windows project

b33dd0d

Fix for windows check of variable type conversion

40af82c

Fix for build error in windows

15a42b2

Make pretty

f6488f7

nibanks approved these changes Oct 17, 2016

View reviewed changes

Remove settings driver from windows kernel mode

f66018b

jwhui approved these changes Oct 17, 2016

View reviewed changes

jwhui merged commit 8468ffd into openthread:master Oct 17, 2016

lluwang mentioned this pull request Oct 17, 2016

CC2538 flash driver that supports non-volatile setting API #325

Closed

jwhui mentioned this pull request Oct 17, 2016

platform: add non-volatile, flash, and/or secured memory driver #30

Closed

Settings Driver #775

Settings Driver #775

Conversation

lluwang commented Oct 8, 2016

nibanks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 9, 2016 • edited

Current coverage is 73.07% (diff: 86.76%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwhui Oct 9, 2016 • edited

Choose a reason for hiding this comment

nibanks commented Oct 9, 2016

jwhui commented Oct 9, 2016

lluwang commented Oct 9, 2016

jwhui commented Oct 9, 2016

jwhui commented Oct 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lluwang Oct 13, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lluwang commented Oct 13, 2016 • edited

jwhui commented Oct 13, 2016

lluwang commented Oct 13, 2016

Choose a reason for hiding this comment

nibanks commented Oct 14, 2016

nibanks commented Oct 14, 2016

lluwang commented Oct 14, 2016

nibanks commented Oct 14, 2016

nibanks commented Oct 14, 2016

jwhui commented Oct 14, 2016 • edited

lluwang commented Oct 17, 2016

nibanks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwhui commented Oct 17, 2016

codecov-io commented Oct 9, 2016 •

edited

jwhui Oct 9, 2016 •

edited

lluwang Oct 13, 2016 •

edited

lluwang commented Oct 13, 2016 •

edited

jwhui commented Oct 14, 2016 •

edited