adafruit / Adafruit_Mynewt

Apache Mynewt documentation and test project for the nRF5x family of BLE SoCs
MIT License
8 stars 3 forks source link

Add NFFS Section for Config Data #8

Closed microbuilder closed 8 years ago

microbuilder commented 8 years ago

Config settings should be stored via nffs and the config system that uses a single NFFS file to store various config values, providing read and write helpers to access values in the config file.

By default, the nRF52 has the following flash memory layout in Mynewt as of 0.9.0 (os_bsp.c):

Some examples of how this is used in practice can be seen here.

We should migrate most projectconfig.h settings to this config file to store everything in one location, and make sure we have a safe 'default' config value when we perform a factory reset.

See the slinky example for a system that uses nffs and config.

microbuilder commented 8 years ago

Rudimentary NFFS support added via https://github.com/adafruit/Adafruit_Mynewt/commit/6683e3441d3d6d1d0f0c4cfe791e0af7ed61c62f

This simply initialises NFFS memory, but when connecting to the shell over serial you will have access to some basic commands to work with the NFFS partition (list files, etc.).

hathach commented 8 years ago

It will be much easier to have an example that use these setting config. Do you think of anything where the setting is saved and used. current bleuart & throughput app does not need such as things.

microbuilder commented 8 years ago

Lets track the HW ID of the last Central device that we connected to for debugging purposes, just to have something. Every time we connect to a central, write the ID of the Central to a config setting in flash. It's not a great example, but until we have a more complex project and some drivers to store settings for it's an easy place to start.

Once we have some drivers installed for things like the 128x32 OLED (see #12) we can store settings for the devices and load them at startup.

hathach commented 8 years ago

I found that newt has built-in support for config file

as far as I know it is something like

name=adafruit
para1=value

Which is a convenient way to save and load settings. I will try to add a devname shell command to change the adv name and saved to the config file. It is possibly closer to what we want to do later on.

hathach commented 8 years ago

it is a bit complicated >.< , still following the code

microbuilder commented 8 years ago

Yes, this is what I want added in. :) It uses a single NFFS file entry and you access the config values with folders and paths like /config/sensors/tsl2561/autolevels via sone helpers. There is an example in the very first post in this issue, but I agree it is a bit complicated at first.

microbuilder commented 8 years ago

It looks like the file moved, but some examples can be seen here as well: https://github.com/apache/incubator-mynewt-core/search?utf8=%E2%9C%93&q=conf_set_value

hathach commented 8 years ago

function pointers passing back and forth, but I almost get there :D . Too bad it is not documented just yet

microbuilder commented 8 years ago

I'm pretty disappointed with the documentation on everything, yes. :( It's just doxygen type skeletons everywhere.

These units tests should be helpful though: https://github.com/apache/incubator-mynewt-core/blob/28e2a8b4efcda9d241720f25749137de37ad71e8/sys/config/src/test/conf_test.c#L315

hathach commented 8 years ago

Some notes for myself.

sensor & blegroup need separated conf_handler ... more to explorer. Sorry for the delay, I like to make it quick by pasting example code. But the config is a bit more complicated, and we need to make sure we have the correct workflow to get it done well.

microbuilder commented 8 years ago

I did start making some notes here, which is the right place to add things, but there isn't much information there at present: https://github.com/adafruit/Adafruit_Mynewt/blob/master/docs/ConfigMemory.md

hathach commented 8 years ago

yeah, I plan to add an shell command to update & dump the config file, making it easier to work with.

microbuilder commented 8 years ago

I think they might already have one, but I'm not sure.

microbuilder commented 8 years ago

See: https://github.com/apache/incubator-mynewt-core/blob/master/sys/config/src/config_cli.c

hathach commented 8 years ago

superb

hathach commented 8 years ago

Finally I got the big picture, in one config file, there are multiple group, each group needs its own conf_handler. e.g

in fact, I don't think we could save a value without a group e.g

microbuilder commented 8 years ago

Yeah, I think we'll have to define a simple folder structure for the config data so that drivers can store config data in a consistent manner (hw/sensors/tls2561/*), but we can figure that out when I start adding some drivers in.

hathach commented 8 years ago

Jush push the code with minimal support for config file. The bleuart is still very simple, it will read the config adafruit/ble/devname and set the adv name upon initializing. You can test it by config dump then change the value, you must run config save to save it to the flash though. Then power off and power on the board it will last a power lost. It serve as proof of concept, we will need to make some helper to make adding/updating config variables easier later.

config dump               
29517:adafruit/ble/devname = Adafruit Bluefruit
config adafruit/ble/devname thach
config save
microbuilder commented 8 years ago

This is a problem with the mynewt code but you apparently can't use a space?

config adafruit/ble/devname Adafruit Mynewt     
109271:Invalid args
config adafruit/ble/devname "Adafruit Mynewt"
133766:Invalid args
microbuilder commented 8 years ago

Works fine with no space but we should allow the space character to be used perhaps when single or double quotes are present? That might mean a pull request to Mynewt core but not being able to use space will become an issue for us longer term.

screen shot 2016-10-04 at 17 55 56
hathach commented 8 years ago

it is the problem with newt shell. could you try with \ to see if it support escape code, I guess not, it is probably the issue with shell parser. I will check the shell code later to verify

microbuilder commented 8 years ago

It's due to the way they use argv in the handler, but I'm going to close this issue since it works and open a new issue to add support to handle spaces which is somewhat distinct and should result in a pull request.

Thanks for getting this in there though ... this covers almost everything we need for a bare minimum 0.1.0 release other than having to test I2C/SPI/ADC once the HAL rework is finished by the core team I hope this week.

microbuilder commented 8 years ago

I'm a bit surprised that things like this aren't handled automatically in simple read/modify/write situations:

/**
 * Callback from config management to load data from Flash to local variable
 * @param argc
 * @param argv
 * @param val
 * @return
 */
static int cfg_set (int argc, char **argv, char *val)
{
  if ( (argc == 2) && !strcmp(argv[0], "ble") && !strcmp(argv[1], "devname") )
  {
    return CONF_VALUE_SET(val, CONF_STRING, ada_cfg.ble_devname);
  }

  return OS_ENOENT;
}

By including the data type along with the entry it should have been possible to automatically retrieve the values and assign it to val, etc.

Similarly for cfg_export and cfg_get, every time we add a new config value we need to handle it ourselves in code. That seems error prone and wastes .text space, although having a standard config API and system is still of course nice, especially since you can write directly to flash for chips like the nRF52 that have no internal EEPROM.

Perhaps the config code should be moved to a separate .c/.h file though to keep this in one place and make it easier to reference elsewhere?

microbuilder commented 8 years ago

See also #26

hathach commented 8 years ago

Yeah, it is indeed, we will need to implement an lookup table much like what we did with SDEP/AT command. newt make it a bit worse when tokenize the adafruit/ble/devname into [adafruit, ble, devname] making matching the variable a bit more works. But it should be easy enough. I also intend to move all the handling code into adautil. Application only need to declare variable + their name then the lib will does the trick.