Linking error (static members) in AnalogSensor.h

[JavierAder]

Device

Lolin

Version

1.18

Bug description

Hello. I wrote a subclass of NTCSensor and when compiling the linker it throws an error:

Linking .pio\build\nodemcu-lolin-analog-mux\firmware.elf
c:/users/javier/.platformio/packages/toolchain-xtensa/bin/../lib/gcc/xtensa-lx106-elf/4.8.2/../../../../xtensa-lx106-elf/bin/ld.exe: .pio/build/nodemcu-lolin-analog-mux/src/sensor.cpp.o:C:\Users\Javier\Documents\PlatformIO\Projects\espurna-1.18.0\code/espurna\sensors/AnalogMux.h:37: undefined reference to `AnalogSensor::SamplesMin'
c:/users/javier/.platformio/packages/toolchain-xtensa/bin/../lib/gcc/xtensa-lx106-elf/4.8.2/../../../../xtensa-lx106-elf/bin/ld.exe: .pio/build/nodemcu-lolin-analog-mux/src/sensor.cpp.o:C:\Users\Javier\Documents\PlatformIO\Projects\espurna-1.18.0\code/espurna\sensors/AnalogMux.h:37: undefined reference to `AnalogSensor::SamplesMax'
collect2.exe: error: ld returned 1 exit status

This doesn't happen if you build a project that uses NTCSensor, but it does if you use a subclass of it (I have no idea why). The solution I found was to add these two lines to the end of AnalogSensor.h

constexpr size_t AnalogSensor::SamplesMin; 
constexpr size_t AnalogSensor::SamplesMax;

Best regards

Steps to reproduce

No response

Build tools used

No response

Any relevant log output (when available)

No response

Decoded stack trace (when available)

No response

[JavierAder]

Probably add also
constexpr int AnalogSensor::RawBits;
is necessary; my actual NTCSensor subclass is reporting SENSOR_ERROR_OVERFLOW, which is set in NTCSensor::pre() and use RawMax
const double voltage = static_cast(_rawRead()) / AnalogSensor::RawMax;
and RawMax is defined using RawBits in AnalogSensor
static constexpr double RawMax { (1 << RawBits) - 1 };
Probably RawMax is getting a random value

[JavierAder]

Probably add also constexpr int AnalogSensor::RawBits; is necessary; my actual NTCSensor subclass is reporting SENSOR_ERROR_OVERFLOW, which is set in NTCSensor::pre() and use RawMax const double voltage = static_cast(_rawRead()) / AnalogSensor::RawMax; and RawMax is defined using RawBits in AnalogSensor static constexpr double RawMax { (1 << RawBits) - 1 }; Probably RawMax is getting a random value

No, surely SENSOR_ERROR_OVERFLOW occurs because readAnalog is returning 65535 instead of 1023. In the debug terminal:

adc 
value 65535
+OK
[319002] [SENSOR] Could not read from NTCMux 0 3.30 - Value Overflow
adc 0
value 65535
+OK

I don't know if it's another bug or my error in the configuration; It seems that my Lolin is starting with the ADC in "Measure the power supply voltage of VDD3P3 (Pin3 and Pin4" mode, although I am not totally sure. In any case, the latter is independent of the linking error.
Why analogRead(0) is returning 65535?

[mcspr]

re. static constexpr declaration / definition outside the class - just a language implementation oddity due to older GCC version and -std=c++11. It is a bug that AnalogSensor.h does not have those two, though.
edit: -std=c++17 and latter makes these redundant, so building w/ -latest or -git envs would work even without the extra definition lines. per https://en.cppreference.com/w/cpp/language/inline#Notes __cpp_inline_variables is the feature-flag to check for non-compatible environments

re. ADC, I would guess it is due to this not being picked up in the config code/espurna/config/sensors.h

espurna/code/espurna/config/sensors.h

Line 1529 in dc9ba63

#define ADC_MODE_VALUE ADC_TOUT

[JavierAder]

re. static constexpr declaration / definition outside the class - just a language implementation oddity due to older GCC version and -std=c++11. It is a bug that AnalogSensor.h does not have those two, though. edit: -std=c++17 and latter makes these redundant, so building w/ -latest or -git envs would work even without the extra definition lines. per https://en.cppreference.com/w/cpp/language/inline#Notes __cpp_inline_variables is the feature-flag to check for non-compatible environments

So, what would be the best solution?

re. ADC, I would guess it is due to this not being picked up in the config code/espurna/config/sensors.h

espurna/code/espurna/config/sensors.h

Line 1529 in dc9ba63

#define ADC_MODE_VALUE ADC_TOUT

Yes, that was it! Thanks!

[JavierAder]

Btw, I had similar linking problems trying to create a Singleton (AnalogMux: #2620 (comment) ). The static method (Inst()) to access the single shared instance of AnalogMux must be defined outside of the class (coming from Java I couldn't understand this restriction, but it seems to be necessary in C++), otherwise linking errors are generated.
Why? I don't know; I am not a expert C++ programmer, but that is suggests for example in https://refactoring.guru/es/design-patterns/singleton/cpp/example

[mcspr]

So, what would be the best solution?

^ 0976bdb (assuming CI passes and I don't have to revert it for some reason)

The static method (Inst()) to access the single shared instance of AnalogMux must be defined outside of the class (coming from Java I couldn't understand this restriction, but it seems to be necessary in C++), otherwise linking errors are generated.

The basic rule here is order of initialization, since static is still effectively in a global scope. Compiler is pretty bad at explaining as to why it wants this symbol present, and does not do it automatically in the global scope.

Order matters in the .cpp -> .o, order of class members, so the order of static members / global static variables, so is the place where T Foo::bar = nullptr; is actually happening in relation to any other object being initialized

ref.
https://en.cppreference.com/w/cpp/language/static#Static_data_members (case above; note that code-block is referring to C++17)
https://en.cppreference.com/w/cpp/language/static (overall)
https://en.cppreference.com/w/cpp/language/siof
(and cppreference wiki in general, even though it is a fairly dry read)