Allow choice settings in syscfg

[andrzej-kaczmarek]

This adds support to have syscfg settings which can be set to one of predefined values. This allows to simplify some settings which now have separate syscfg defined for each choice. With this new setting
newt can guarantee that only single setting for selected choice is configured in syscfg.h.

I'm not sure if this is good approach, so perhaps someone has better idea how to achieve the same result. The main disadvantage of current implementation is that it breaks compatibility with old newt versions since they do not generate MYNEWT_VAL_CHOICE macro in syscfg.h which is a helper to check if choice is selected. Anyway, let me explain how this works (snippets below are from apache/mynewt-core#1654)

Sample settings looks like this:

    MCU_LFCLK_SOURCE:
        description: >
            Selected source for low frequency clock (LFCLK).
        value:
        choices:
            - LFRC      # 32.768 kHz RC oscillator
            - LFXO      # 32.768 kHz crystal oscillator
            - LFSYNTH   # 32.768 kHz synthesized from HFCLK

If choices key is present, value has to be either empty or one of specified choices. It is valid to leave value empty, thus $notnull must be used if setting requires a valid value. List of choices is a list of strings (letters, numbers and underscores allowed) and internally they are handled case-insensitive (thus 'LFRCand'lfrc` would be considered the same choice which is invalid).

Assuming MCU_TARGET: nrf52840, setting defined as above will generate following defines in syscfg.h:

/* Overridden by @apache-mynewt-core/hw/bsp/nordic_pca10056 (defined by @apache-mynewt-core/hw/mcu/nordic/nrf52xxx) */
#ifndef MYNEWT_VAL_MCU_LFCLK_SOURCE__LFRC
#define MYNEWT_VAL_MCU_LFCLK_SOURCE__LFRC (0)
#endif
#ifndef MYNEWT_VAL_MCU_LFCLK_SOURCE__LFSYNTH
#define MYNEWT_VAL_MCU_LFCLK_SOURCE__LFSYNTH (0)
#endif
#ifndef MYNEWT_VAL_MCU_LFCLK_SOURCE__LFXO
#define MYNEWT_VAL_MCU_LFCLK_SOURCE__LFXO (1)
#endif
#ifndef MYNEWT_VAL_MCU_LFCLK_SOURCE
#define MYNEWT_VAL_MCU_LFCLK_SOURCE (1)
#endif

Main sysycfg name is ehtier 1 when any value is set or 0 otherwise. Each choice listed have separate define which consists of syscfg name and choice name (in lowercase) set to 1 if this is the selected value or 0 otherwise. To make checks easier, MYNEWT_VAL_CHOICE(_name, _val) macro is defined which checks for given choice on syscfg value. Note that even though value was specified in lower-case, original spelling of choice is preserved in syscfg.h.

[ccollins476ad]

I like the idea, and the code looks good to me. I would like to give this a bit more thought before approving, just in case this feature conflicts with other parts of newt and I haven't realized it.

One thought: Do we really need the user to explicitly state the $validchoice restriction? It would be simpler to implicitly enforce this restriction if there is a choices field.

[wes3]

I was thinking the same thing as Chris re:$validchoice. Not sure why the settings as defined above would produce the output you show. Did you mean to set value: lfxo in the example? Otherwise, all seems good to me

[andrzej-kaczmarek]

Good point, we don't need $validchoice here. I added it at some point and did not notice it's redundant - will remove it.

@wes3 I copied output from apache/mynewt-core#1654 and forgot that it also sets MCU_LFCLK_SOURCE: lfxo in my configuration, so yes - there's one step missing here :-)

[ccollins476ad]

One other question: why do the choices get converted to lower case? I think it would be less confusing if the C code could specify the exact identifier that appears in the YAML file. As it is now, if the identifier contains any capital letters, the C code will have to manually downcase it.

That is just my initial thought, though. I might not be thinking it through very well.

Aside from that, I think it looks good!

[andrzej-kaczmarek]

@ccollins476ad I would say it's for "historical reasons" ;-) Long story short, I wanted to make comparison case-insensitive as having e.g. AA and Aa my be confusing (is it?) and it was just left like this. But that's certainly a good point that C code should use exact identifiers as defined in choices - I will fix that.

Also I'll add check to make sure there are no duplicated choices on the list which is missing here. Should we use case-sensitive or -insensitive comparison here? I'd opt for case-insensitive for the reason I mentioned above, but perhaps there are good reasons why it should be case-sensitive.

[ccollins476ad]

I think it's reasonable to use a case-insensitive compare when checking for duplicates.

[andrzej-kaczmarek]

fixed comments and updated description to describe current behavior

[ccollins476ad]

This comment should probably be changed back to "two".