External libraries support at runtime

[AeroStun]

Officially handle smartcar_shield across all of SMCE's supported platforms Let users add their own libraries to the board config and passthrough that to the CMake invocation

Acceptance criteria: works as expected on all three major platforms and is well documented

[AeroStun]

Turns out smartcar_shield is not ISO C++ compliant, so it will not work on MSVC

[platisd]

Turns out smartcar_shield is not ISO C++ compliant, so it will not work on MSVC

Can I make a change that helps?

[AeroStun]

Can I make a change that helps?

The GNU++ feature you use is VLAs; you use them to make an array of measurement results before sorting it to find the median.

Given this is in a virtual method, the IMO most correct way would be to use a fixed-size buffer and document that there may not be more than the max number of iterations (as well as clamp the user's value for safety); this is AFAIK the most common practice in portable heapless C/C++ Otherwise, you could conditionally use std::vector instead of VLAs when the platform supports it (see __has_include which was standardized in C++17, but I've tested all 3 major compilers and they support it for all C++ standards)

If it wasn't in a virtual method, I'd simply pass the number of iterations as an NTTP and call it a day

[platisd]

I really hate that rule but you're right. I'll probably go with the fixed-sized buffer solution. If we were only targeting the ESP8266 or ESP32 then we'd definitely use vectors but since we also have to support 8-bit microcontrollers, then using two different data structures would just make the code (and more importantly the API/documentation) more complex.

Can you create an issue at the smartcar_shield repo?

[AeroStun]

Out of curiosity, how would it make the code any more complex? The only places where a vector would be treated differently is at the point of declaration and at the point of call to getMedian (to pass .data() through as the pointer)

Heading to your repo now

[platisd]

Let's take the following snippet for example:

unsigned int SR04::getMedianDistance(uint8_t iterations)
{
    if (iterations == 0)
    {
        return kError;
    }

    unsigned int measurements[iterations];
    for (auto i = 0; i < iterations; i++)
    {
        measurements[i] = getDistance();
        mRuntime.delayMillis(kMedianMeasurementDelay);
    }

    return getMedian(measurements, iterations);
}

The problem is with iterations in unsigned int measurements[iterations];

So we'd need an ifdef statement around it, switching between a C-style array and a std::vector. Then, we could probably overload getMedian so that wouldn't be an issue. Moreover, we'd need to conditionally include <vector>, probably where getMedian is implemented. Alternatively, we could get the vector's .data() as you proposed. The amount of conditional compilations doesn't considerably change.

However, the biggest problem is with the API towards the user. Considering that we should always be ISO C++ compliant, even for 8-bit AVRs, we would have to maintain an upper limit, e.g. 255 that we would document and communicate to the user. This limit is however not necessary in the case of std::vector. So we either choose to have a documented limit that is really necessary only for low-end microcontrollers in both platforms or have that limit only for AVR microcontrollers and communicate that accordingly. The latter scenario is according to me bad, since we are leaking implementation and platform details to the users. The former, is not so bad as its transparent to the user. However, if we are going to have such a limit, then what's the point of bothering with a std::vector in the first place?

[AeroStun]

iterations being a uint8_t already documents a limit of std::numeric_limits<uint8_t>::max() (i.e. 255) regardless of the targeted platform and the implementation details. But ya I see your point of "why even bother"

[platisd]

Rather unrelated, but out of curiosity when compiling for Arduino UNO:

sketch_may27a:1:10: error: limits: No such file or directory
 #include <limits>
          ^~~~~~~~
compilation terminated.
exit status 1
limits: No such file or directory

I'll look into the issue you opened soon. :+1:

[AeroStun]

Nice non-compliant compiler you have there https://eel.is/c++draft/compliance#2 CppReference shows <limit> should be there even in C++98 Anyway, good ol' ~static_cast<UnsignedIntegralType>(0) does the trick too to get the max on NonCompliantPlatforms:tm: assuming they at least use bits correctly

Thanks for caring and looking into it! We appreciate it (and our [future] Windows users probably too)

[platisd]

platisd/smartcar_shield#22 is now fixed so it should help with this issue as well.

[AeroStun]

Thanks! Will see what MSVC thinks of it once I finally get the linkers to behave

[AeroStun]

Linkers now behave