Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: overload 'get' in order to enable std::string as supported parameters. #238

Merged
merged 3 commits into from
Jul 6, 2023

Conversation

aentinger
Copy link
Member

As suggest by @pavel-kirienko over here.

Unfortunately leads to a really ugly error message ...

In file included from /home/alex/Arduino/libraries/107-Arduino-Cyphal/src/util/nodeinfo/../../types/uavcan/node/port/ID_1_0.hpp:51,
                 from /home/alex/Arduino/libraries/107-Arduino-Cyphal/src/util/nodeinfo/../../DSDL_Types.h.impl:11,
                 from /home/alex/Arduino/libraries/107-Arduino-Cyphal/src/util/nodeinfo/../../DSDL_Types.h:20,
                 from /home/alex/Arduino/libraries/107-Arduino-Cyphal/src/util/nodeinfo/NodeInfoBase.hpp:17,
                 from /home/alex/Arduino/libraries/107-Arduino-Cyphal/src/Node.hpp:30,
                 from /home/alex/Arduino/libraries/107-Arduino-Cyphal/src/107-Arduino-Cyphal.h:15,
                 from /home/alex/projects/107-systems/OpenCyphalPicoBase-firmware/OpenCyphalPicoBase-firmware.ino:18:
/home/alex/.arduino15/packages/rp2040/tools/pqt-gcc/1.5.0-a-5007782/arm-none-eabi/include/c++/12.2.0/variant: In substitution of 'template<unsigned int _Np, class ... _Args> std::enable_if_t<(!(_Np < 15)), void> std::variant<uavcan::primitive::Empty_1_0, uavcan::primitive::String_1_0, uavcan::primitive::Unstructured_1_0, uavcan::primitive::array::Bit_1_0, uavcan::primitive::array::Integer64_1_0, uavcan::primitive::array::Integer32_1_0, uavcan::primitive::array::Integer16_1_0, uavcan::primitive::array::Integer8_1_0, uavcan::primitive::array::Natural64_1_0, uavcan::primitive::array::Natural32_1_0, uavcan::primitive::array::Natural16_1_0, uavcan::primitive::array::Natural8_1_0, uavcan::primitive::array::Real64_1_0, uavcan::primitive::array::Real32_1_0, uavcan::primitive::array::Real16_1_0>::emplace(_Args&& ...) [with unsigned int _Np = 896; _Args = <missing>]':
/home/alex/Arduino/libraries/107-Arduino-Cyphal/src/util/registry/registry_value.hpp:96:67:   recursively required from 'constexpr const std::size_t registry::detail::ArraySelector<char, 1, void>::Index'
/home/alex/Arduino/libraries/107-Arduino-Cyphal/src/util/registry/registry_value.hpp:96:67:   required from 'constexpr const std::size_t registry::detail::ArraySelector<char, 0, void>::Index'
/home/alex/Arduino/libraries/107-Arduino-Cyphal/src/util/registry/registry_value.hpp:214:57:   required by substitution of 'template<class Container, class T, unsigned int Index> void registry::set(Value&, const Container&) [with Container = std::__cxx11::basic_string<char>; T = char; unsigned int Index = <missing>]'
/home/alex/Arduino/libraries/107-Arduino-Cyphal/src/util/registry/registry_impl.hpp:326:26:   required from 'registry::ValueWithMetadata registry::Registry::Reg<N, G, S, IsMutable>::get() const [with N = std::basic_string_view<char>; G = registry::Registry::expose<const char (&)[24], std::__cxx11::basic_string<char> >(const char (&)[24], const Options&, std::__cxx11::basic_string<char>&)::<lambda()>; S = registry::Registry::expose<const char (&)[24], std::__cxx11::basic_string<char> >(const char (&)[24], const Options&, std::__cxx11::basic_string<char>&)::<lambda(const registry::Value&)>; bool IsMutable = true]'
/home/alex/Arduino/libraries/107-Arduino-Cyphal/src/util/registry/registry_impl.hpp:323:41:   required from here
/home/alex/.arduino15/packages/rp2040/tools/pqt-gcc/1.5.0-a-5007782/arm-none-eabi/include/c++/12.2.0/variant:1586:49: fatal error: template instantiation depth exceeds maximum of 900 (use '-ftemplate-depth=' to increase the maximum)
 1586 |         enable_if_t<!(_Np < sizeof...(_Types))> emplace(_Args&&...) = delete;
      |                                                 ^~~~~~~
compilation terminated.

CC @generationmake

@aentinger aentinger added topic: firmware Code that runs on an embedded system. type: enhancement PR to improve the project. labels Jul 5, 2023
@aentinger aentinger self-assigned this Jul 5, 2023
@pavel-kirienko
Copy link
Member

Ah, interesting. The compiler chose void set(Value& dst, const Container& src) over inline void set(Value& dst, const std::string_view string) because it doesn't require an implicit conversion std::string -> std::string_view. We need to help the compiler choose the correct overload. To do that, we can either use SFINAE, or to add an explicit overload for string. The latter is a bit easier, so please add something roughly like this:

inline void set(Value& dst, const std::string& string)
{
    set(dst, std::string_view(string));
}

@aentinger
Copy link
Member Author

Now explicitly overloading for std::string generates an call of overloaded 'set(registry::Value&, const char*)' is ambiguous error, see err.txt . 😢

@107-systems 107-systems deleted a comment from codecov-commenter Jul 5, 2023
@107-systems 107-systems deleted a comment from github-actions bot Jul 5, 2023
@github-actions
Copy link

github-actions bot commented Jul 5, 2023

Memory usage change @ b9f4393

Board flash % RAM for global variables %
rp2040:rp2040:arduino_nano_connect 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
rp2040:rp2040:rpipico 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
Click for full report table
Board examples/OpenCyphal-Blink
flash
% examples/OpenCyphal-Blink
RAM for global variables
% examples/OpenCyphal-Heartbeat-Publisher
flash
% examples/OpenCyphal-Heartbeat-Publisher
RAM for global variables
% examples/OpenCyphal-Heartbeat-Subscriber
flash
% examples/OpenCyphal-Heartbeat-Subscriber
RAM for global variables
% examples/OpenCyphal-Service-Client
flash
% examples/OpenCyphal-Service-Client
RAM for global variables
% examples/OpenCyphal-Service-Server
flash
% examples/OpenCyphal-Service-Server
RAM for global variables
%
rp2040:rp2040:arduino_nano_connect 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0
rp2040:rp2040:rpipico 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0
Click for full report CSV
Board,examples/OpenCyphal-Blink<br>flash,%,examples/OpenCyphal-Blink<br>RAM for global variables,%,examples/OpenCyphal-Heartbeat-Publisher<br>flash,%,examples/OpenCyphal-Heartbeat-Publisher<br>RAM for global variables,%,examples/OpenCyphal-Heartbeat-Subscriber<br>flash,%,examples/OpenCyphal-Heartbeat-Subscriber<br>RAM for global variables,%,examples/OpenCyphal-Service-Client<br>flash,%,examples/OpenCyphal-Service-Client<br>RAM for global variables,%,examples/OpenCyphal-Service-Server<br>flash,%,examples/OpenCyphal-Service-Server<br>RAM for global variables,%
rp2040:rp2040:arduino_nano_connect,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0
rp2040:rp2040:rpipico,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0

@pavel-kirienko
Copy link
Member

That's also easy -- add another overload:

inline void set(Value& dst, const char* const string)
{
    set(dst, std::string_view(string));
}
``

@aentinger
Copy link
Member Author

Thank you @pavel-kirienko 🙇 . This compiles now, I'll ask @generationmake to perform some tests.

@github-actions
Copy link

github-actions bot commented Jul 5, 2023

Memory usage change @ d69d9a6

Board flash % RAM for global variables %
rp2040:rp2040:arduino_nano_connect 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
rp2040:rp2040:rpipico 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
Click for full report table
Board examples/OpenCyphal-Blink
flash
% examples/OpenCyphal-Blink
RAM for global variables
% examples/OpenCyphal-Heartbeat-Publisher
flash
% examples/OpenCyphal-Heartbeat-Publisher
RAM for global variables
% examples/OpenCyphal-Heartbeat-Subscriber
flash
% examples/OpenCyphal-Heartbeat-Subscriber
RAM for global variables
% examples/OpenCyphal-Service-Client
flash
% examples/OpenCyphal-Service-Client
RAM for global variables
% examples/OpenCyphal-Service-Server
flash
% examples/OpenCyphal-Service-Server
RAM for global variables
%
rp2040:rp2040:arduino_nano_connect 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0
rp2040:rp2040:rpipico 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0
Click for full report CSV
Board,examples/OpenCyphal-Blink<br>flash,%,examples/OpenCyphal-Blink<br>RAM for global variables,%,examples/OpenCyphal-Heartbeat-Publisher<br>flash,%,examples/OpenCyphal-Heartbeat-Publisher<br>RAM for global variables,%,examples/OpenCyphal-Heartbeat-Subscriber<br>flash,%,examples/OpenCyphal-Heartbeat-Subscriber<br>RAM for global variables,%,examples/OpenCyphal-Service-Client<br>flash,%,examples/OpenCyphal-Service-Client<br>RAM for global variables,%,examples/OpenCyphal-Service-Server<br>flash,%,examples/OpenCyphal-Service-Server<br>RAM for global variables,%
rp2040:rp2040:arduino_nano_connect,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0
rp2040:rp2040:rpipico,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0

@aentinger aentinger merged commit 53095a0 into main Jul 6, 2023
16 checks passed
@aentinger aentinger deleted the fix-no-string-support branch July 6, 2023 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: firmware Code that runs on an embedded system. type: enhancement PR to improve the project.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants