Re: [riot-devel] Compile-time peripheral configuration in board definitions

2018-12-17 Thread Gunar Schorcht
Hi Joakim,

many thanks for your comprehensive answer.

> First, let's clear up one thing regarding the current
> implementation: The static const structs defined in periph_conf.h are
> garbage collected in all modules where they are not directly used, so
>  usually, this means that it will only use ROM once (in the .rodata 
> section of the appropriate periph driver object). You can see this in
> the .map file in the bin dir after building, there should only be one
> reference to each configuration struct.

Yes, that's what I meant when I said "Each object file would have an
own copy of this static variable if it is used." I should have added
"and only if it is used" to clarify that.

> I don't know if using the _NUMOF macro constitutes as usage with 
> regards to linker GC, if the compiler is smart then the struct itself
> should not be allocated when you only use sizeof(pwm_conf) but never
> reference pwm_conf itself.

I would guess that the `sizeof(array/array[0])` can be determined during
compile time and the structure is not allocated.

> Putting the configuration struct in the periph driver implementation 
> file is equivalent to putting it in the header from the compiler 
> point of view when building the periph driver, so it should be 
> possible to perform the same optimizations as when the configuration
>  is in periph_conf.h, which is good.

Provided that it is used in periph module only.

> Putting the configuration in the periph driver implementation will 
> place the configuration inside the CPU, which is bad for flexibility 
> and configurability.
> 
> Unless you also define an initializer macro somewhere in the board 
> config, e.g. periph_conf.h, which is a bit more inconvenient than the
> current approach due to multi line macros needing to escape newlines
> etc.

You are right, but there should be only configurable parts exposed in
header file. For example, for the ESP32 PWM, configuration details such
as the periphery module used, the register set used, the signal group
used, or the interrupt source used must not be changed by board
definition and should not be exposed to the header file. In case of
ESP32 PWM, only the pins used as PWM channels are configurable which is
exposed as a simple list of GPIO pins.

My impression is, if I'm not wrong, that some MCUs expose not only
configuration details but also implementation details in their `*_conf`
structs.

> One possible alternative solution is to create a periph_conf.c file 
> in the board directory, and use extern declarations in periph_conf.h 
> just like you proposed in order to let the drivers access the

It was not a proposal but just a description how it is realized for
ESP32 boards at the moment. I'm just looking for some advise on whether
to change to the usual approach used in RIOT for other boards.

> structs. The drawback here is that the compiler will not be able to 
> optimize away execution paths which are not reachable using the 
> current configuration. On some implementations this will make no 
> difference but on others this will make the code a lot larger due to 
> multiple large branches which would normally be optimized away due
> to dead code elimination in the compiler.

In the case of ESP32, if I place the static arrays of PWM pins in the
header file instead of defining them as macros, the code gets a bit larger.

> XFA would also have the same drawback as the periph_conf.c solution,
> because the whole array is not visible to the compiler during
> compilation of periph/abcd.c, so it can't fully perform DCE and other
> optimizations.

Ah ok, I understand.

To summarize your e-mail, you would advise to change the definitions of
ESP32 boards to expose configuration structures in header files
according to the RIOT approach for board definitions.

Regards
Gunar

> Best regards, Joakim
> 
> On Mon, Dec 17, 2018 at 4:14 PM Gunar Schorcht  
> wrote:
>> 
>> Hi,
>> 
>> I'm thinking about to revise the ESP32 board definitions and would
>>  need an advice from experienced core developers.
>> 
>> Most (if not all) boards define their peripheral configuration of 
>> ADCs, DACs and PWMs in `periph_conf.h` in a kind of static const 
>> arrays and define macros based on the size of these arrays as 
>> follows:
>> 
>> ``` static const pwm_conf_t pwm_conf[] = { { .dev = MINI_TIMER0, 
>> .pin_ch = PWM_PINS_CH0, .div = MINI_TIMER0_DIV, }, { .dev = 
>> MINI_TIMER2, .pin_ch = PWM_PINS_CH1, .div = MINI_TIMER2_DIV, } }; 
>> #define PWM_NUMOF (sizeof(pwm_conf) / sizeof(pwm_conf[0])) ```
>> 
>> Even though these static const arrays are allocated in the .rodata 
>> segment, they are allocated in each module that includes `board.h`.
>> XFAs addresses this problem.
>> 
>> For ESP32-Boards I tried another approach. In `periph_conf.h` there
>> is only a declaration of an external const variable `pwm_dev_num`
>> which contains the number of PWM devices and the macro `PWM_NUMOF`
>> is then defined as follows:
>> 
>> ``` extern const unsigned 

Re: [riot-devel] Compile-time peripheral configuration in board definitions

2018-12-17 Thread Joakim Nohlgård
Hi Gunar,
I have experimented with something similar, moving the configurations
to a .c file instead of periph_conf.h, but I came to the conclusion
that there are a number of drawbacks to the various alternatives.

First, let's clear up one thing regarding the current implementation:
The static const structs defined in periph_conf.h are garbage
collected in all modules where they are not directly used, so usually,
this means that it will only use ROM once (in the .rodata section of
the appropriate periph driver object). You can see this in the .map
file in the bin dir after building, there should only be one reference
to each configuration struct.
I don't know if using the _NUMOF macro constitutes as usage with
regards to linker GC, if the compiler is smart then the struct itself
should not be allocated when you only use sizeof(pwm_conf) but never
reference pwm_conf itself.

Putting the configuration struct in the periph driver implementation
file is equivalent to putting it in the header from the compiler point
of view when building the periph driver, so it should be possible to
perform the same optimizations as when the configuration is in
periph_conf.h, which is good.
Putting the configuration in the periph driver implementation will
place the configuration inside the CPU, which is bad for flexibility
and configurability. Unless you also define an initializer macro
somewhere in the board config, e.g. periph_conf.h, which is a bit more
inconvenient than the current approach due to multi line macros
needing to escape newlines etc.
One possible alternative solution is to create a periph_conf.c file in
the board directory, and use extern declarations in periph_conf.h just
like you proposed in order to let the drivers access the structs. The
drawback here is that the compiler will not be able to optimize away
execution paths which are not reachable using the current
configuration. On some implementations this will make no difference
but on others this will make the code a lot larger due to multiple
large branches which would normally be optimized away due to dead code
elimination in the compiler.
XFA would also have the same drawback as the periph_conf.c solution,
because the whole array is not visible to the compiler during
compilation of periph/abcd.c, so it can't fully perform DCE and other
optimizations.

Best regards,
Joakim

On Mon, Dec 17, 2018 at 4:14 PM Gunar Schorcht  wrote:
>
> Hi,
>
> I'm thinking about to revise the ESP32 board definitions and would need
> an advice from experienced core developers.
>
> Most (if not all) boards define their peripheral configuration of ADCs,
> DACs and PWMs in `periph_conf.h` in a kind of static const arrays and
> define macros based on the size of these arrays as follows:
>
> ```
> static const pwm_conf_t pwm_conf[] = {
> {
> .dev = MINI_TIMER0,
> .pin_ch = PWM_PINS_CH0,
> .div = MINI_TIMER0_DIV,
> },
> {
> .dev = MINI_TIMER2,
> .pin_ch = PWM_PINS_CH1,
> .div = MINI_TIMER2_DIV,
> }
> };
> #define PWM_NUMOF (sizeof(pwm_conf) / sizeof(pwm_conf[0]))
> ```
>
> Even though these static const arrays are allocated in the .rodata
> segment, they are allocated in each module that includes `board.h`. XFAs
> addresses this problem.
>
> For ESP32-Boards I tried another approach. In `periph_conf.h` there is
> only a declaration of an external const variable `pwm_dev_num` which
> contains the number of PWM devices and the macro `PWM_NUMOF` is then
> defined as follows:
>
> ```
> extern const unsigned pwm_dev_num;
> #define PWM_NUMOF   (pwm_dev_num)
> ```
>
> That's it. The complex configuration array of PWM is defined as static
> const array in the according .c file. The `prm_dev_num` variable is also
> defined there as usual:
> ```
> const unsigned pwm_dev_num = sizeof(_pwm_hw) / sizeof(_pwm_hw[0]);
> ```
>
> With this apporach, the pretty complex static configuration array
> `_pwm_hw[]` exists only once, the complexity of the configuration is
> hidden and the variable `pwm_dev_num` is also determin at compile time.
>
> The question I have is: should I change the board definitions according
> to the approach used by all other board definitions to be compatible
> with these board definitions, or can I leave them as they are?
>
> What would be your advice?
>
> Regards
> Gunar
>
> --
> Wenn du laufen willst, lauf eine Meile. Wenn du ein neues Leben
> kennenlernen willst, dann lauf Marathon. (Emil Zatopek)
>
> ___
> devel mailing list
> devel@riot-os.org
> https://lists.riot-os.org/mailman/listinfo/devel
___
devel mailing list
devel@riot-os.org
https://lists.riot-os.org/mailman/listinfo/devel


Re: [riot-devel] Compile-time peripheral configuration in board definitions

2018-12-17 Thread Gunar Schorcht
Hi Juan,

thanks for your answer.

> So you are saying that the data gets duploicated in ROM several 
> times? I did not know that. If that is the case, then it's
> definitely a bad thing!

Yes, at least for object files where they are used and cannot be
optimized. The static storage type limits the scope of a variable to a
particular file. If you define such a static variable wihtin a header
file, it becomes a static variable definition with limited scope in each
.c file that includes this header file. Each object file would have an
own copy if this variable if it is used.

Regards
Gunar


-- 
Dr. Gunar Schorcht

Mission Level Design GmbH
Langewiesener Strasse 22
98693 Ilmenau

schor...@mldesigner.de

Sitz der Gesellschaft:
Lohmühlenweg 2
99310 Arnstadt

Amtsgericht Jena: HRB 111384
Geschäftsführung: Dipl.-Inf. Tino Jungebloud

-- 
Wenn du laufen willst, lauf eine Meile. Wenn du ein neues Leben
kennenlernen willst, dann lauf Marathon. (Emil Zatopek)



signature.asc
Description: OpenPGP digital signature
___
devel mailing list
devel@riot-os.org
https://lists.riot-os.org/mailman/listinfo/devel


Re: [riot-devel] Compile-time peripheral configuration in board definitions

2018-12-17 Thread Juan Ignacio Carrano

Hi Gunar,


Even though these static const arrays are allocated in the .rodata
segment, they are allocated in each module that includes `board.h`. XFAs
addresses this problem.



So you are saying that the data gets duploicated in ROM several times? I 
did not know that. If that is the case, then it's definitely a bad thing!



For ESP32-Boards I tried another approach. In `periph_conf.h` there is
only a declaration of an external const variable `pwm_dev_num` which
contains the number of PWM devices and the macro `PWM_NUMOF` is then
defined as follows:

```
extern const unsigned pwm_dev_num;
#define PWM_NUMOF   (pwm_dev_num)
```



Regardless of ROM duplication or not, I think you approach is much 
better and cleaner. Also, you don't bloat the header, and don't have the 
compiler parse the same definitions several times. I see no reasons to 
define the array in the header file.


Regards,

Juan.
___
devel mailing list
devel@riot-os.org
https://lists.riot-os.org/mailman/listinfo/devel


[riot-devel] Compile-time peripheral configuration in board definitions

2018-12-17 Thread Gunar Schorcht
Hi,

I'm thinking about to revise the ESP32 board definitions and would need
an advice from experienced core developers.

Most (if not all) boards define their peripheral configuration of ADCs,
DACs and PWMs in `periph_conf.h` in a kind of static const arrays and
define macros based on the size of these arrays as follows:

```
static const pwm_conf_t pwm_conf[] = {
{
.dev = MINI_TIMER0,
.pin_ch = PWM_PINS_CH0,
.div = MINI_TIMER0_DIV,
},
{
.dev = MINI_TIMER2,
.pin_ch = PWM_PINS_CH1,
.div = MINI_TIMER2_DIV,
}
};
#define PWM_NUMOF (sizeof(pwm_conf) / sizeof(pwm_conf[0]))
```

Even though these static const arrays are allocated in the .rodata
segment, they are allocated in each module that includes `board.h`. XFAs
addresses this problem.

For ESP32-Boards I tried another approach. In `periph_conf.h` there is
only a declaration of an external const variable `pwm_dev_num` which
contains the number of PWM devices and the macro `PWM_NUMOF` is then
defined as follows:

```
extern const unsigned pwm_dev_num;
#define PWM_NUMOF   (pwm_dev_num)
```

That's it. The complex configuration array of PWM is defined as static
const array in the according .c file. The `prm_dev_num` variable is also
defined there as usual:
```
const unsigned pwm_dev_num = sizeof(_pwm_hw) / sizeof(_pwm_hw[0]);
```

With this apporach, the pretty complex static configuration array
`_pwm_hw[]` exists only once, the complexity of the configuration is
hidden and the variable `pwm_dev_num` is also determin at compile time.

The question I have is: should I change the board definitions according
to the approach used by all other board definitions to be compatible
with these board definitions, or can I leave them as they are?

What would be your advice?

Regards
Gunar

-- 
Wenn du laufen willst, lauf eine Meile. Wenn du ein neues Leben
kennenlernen willst, dann lauf Marathon. (Emil Zatopek)



signature.asc
Description: OpenPGP digital signature
___
devel mailing list
devel@riot-os.org
https://lists.riot-os.org/mailman/listinfo/devel