UncleRus / esp-idf-lib

Component library for ESP32-xx and ESP8266
https://esp-idf-lib.readthedocs.io/en/latest/
1.35k stars 420 forks source link

fix: lsm303 ODR names and non-static scale arrays #573

Closed QB4-dev closed 11 months ago

QB4-dev commented 11 months ago

I have fixed ODR enum names and removed static specifier conversion arrays

UncleRus commented 11 months ago

removed static specifier conversion arrays

Why?

QB4-dev commented 11 months ago

Previously I have kept them as global variables outside of functions so added static for not being visible outside .c files, and forgot to remove static when pasted inside the functions.

All calculations using these arrays are done inside the functions so I think it is OK to keep them on stack with const specifier

Tell me if I am wrong, driver still works fine

UncleRus commented 11 months ago

I don't see the need to store constants on the stack. I could understand the need for this, for example, in case of optimization of heavy-loaded calculations, but here it is just completely unnecessary stack consumption.

UncleRus commented 11 months ago

And you don't need to define module-level static constants if you don't want to pollute the module-level namespace. Just declare them as static constants directly in the function (as it is done now) and then these constants will have all the properties declared at the module level, but they will be visible only from the function itself.

static const int module_level_decl = 0; // visible in both func() and func2()

void func()
{
    static const int func_level_decl = 0;  // same properties as module_level_decl, but visible only in func()
}

void func2()
{
    ...
}
QB4-dev commented 11 months ago

It seems that const is enough to place array not on stack:

https://godbolt.org/z/337M3rzvP

See assembly output when static is removed, and then remove const - then array will be allocated on stack.

So I was wrong. With const stack mem is not consumed

UncleRus commented 11 months ago

OK, for consts, there is no need to specify static for placement not on the stack. However, the explicit is better than the implicit, and it is more convenient for me to read the source code if the constants are static.

UncleRus commented 11 months ago

https://godbolt.org/z/337M3rzvP

Cool service, BTW. Thanks!

QB4-dev commented 11 months ago

Cool service, BTW. Thanks!

You are welcome. Added static specifier back

UncleRus commented 11 months ago

Thank you!