openmrn icon indicating copy to clipboard operation
openmrn copied to clipboard

MultiConfigurePC not usable?

Open RobertPHeller opened this issue 3 years ago • 15 comments

Since the standard thing for Gpio * classes is to use the GpioWrapper class and since the GpioWrapper class does not allow changing direction, this means that one cannot have Gpio Lines that can be configured as input or output, this means that MultiConfigurePC cannot be used. Either it should be removed or GpioWrapper needs to be replaced or possibly something else.

Using the MultiConfigurePC config causes a run-time crash due to the HASSERTs in the set_direction() method of the GpioWrapper class.

RobertPHeller avatar Oct 26 '21 13:10 RobertPHeller

I've used the MultiConfigurePC object successfully on the ESP32-S2 (though it should work on others), I encountered the same limitation and modified the Esp32Gpio code to not use GpioWrapper.

atanisoft avatar Oct 26 '21 13:10 atanisoft

OK, I will look at Esp32Gpio and see if I can make a similar modification to LinuxGpio...

I wonder: if all of the *Gpio implementations are eventually modified to not use GpioWrapper, what happens to GpioWrapper? -- I wonder if GpioWrapper's set_direction() is plain wrong -- eg why does it have the "restriction" against changing direction?

RobertPHeller avatar Oct 26 '21 13:10 RobertPHeller

I believe most of the Gpio implementations have already moved off GpioWrapper already.

atanisoft avatar Oct 26 '21 13:10 atanisoft

OK, so GpioWrapper is effectively depreciated?

RobertPHeller avatar Oct 26 '21 13:10 RobertPHeller

I can't say for certain on that, it is still used by a handful of target (STM32 being one).

atanisoft avatar Oct 26 '21 13:10 atanisoft

I think the question is if we add set_direction to GpioWrapper then what happens when it is used on a platform whose GPIO class that does not have a changeable direction?

If someone can put together the magic template arguments (std::enable_if<...>) then we should be fine.

balazsracz avatar Oct 26 '21 14:10 balazsracz

std::enable_if is only available in C++14 which is not available by default in arduino-esp32 (possibly other platforms). We will need a fallback option when using C++11 (which is really old!)

atanisoft avatar Oct 26 '21 14:10 atanisoft

https://en.cppreference.com/w/cpp/types/enable_if since c++11

balazsracz avatar Oct 26 '21 14:10 balazsracz

Ahh, I landed on a diff page that stated C++14. If we can craft the necessary magic we can certainly go that route.

atanisoft avatar Oct 26 '21 14:10 atanisoft

I have recoded LinuxGpio to not use GpioWrapper (using Esp32Gpio as a model), but I am getting a compile error:

/include -I. -I/home/heller/RRCircuits/MegaIOOpenMRN --std=c++11 -MD -MF compile_cdi.d /home/heller/openmrn/src/openlcb/CompileCdiMain.cxx In file included from ./Hardware.hxx:4, from ./config.hxx:10, from /home/heller/openmrn/src/openlcb/CompileCdiMain.cxx:6: /home/heller/openmrn/src/os/LinuxGpio.hxx:176:46: error: template definition of non-template ‘const LinuxGpio<PIN_NUM> LinuxGpio<PIN_NUM>::instance_’ const LinuxGpio<PIN_NUM> LinuxGpio<PIN_NUM>::instance_; ^~~~~~~~~

Code is pushed to my fork: https://github.com/RobertPHeller/openmrn/blob/LinuxGpio-no-GpioWrapper/src/os/LinuxGpio.hxx

RobertPHeller avatar Oct 26 '21 15:10 RobertPHeller

https://github.com/RobertPHeller/openmrn/blob/LinuxGpio-no-GpioWrapper/src/os/LinuxGpio.hxx#L93 declares the template type as "int". https://github.com/RobertPHeller/openmrn/blob/LinuxGpio-no-GpioWrapper/src/os/LinuxGpio.hxx#L175 defines the type as "gpio_num_t" (define to unsigned int).

I'd suggest drop all usages of gpio_num_t and use int or even uint8_t if you can fit the IO pin count in that size.

atanisoft avatar Oct 26 '21 16:10 atanisoft

also it would be better to use typedef instead of define for types...

atanisoft avatar Oct 26 '21 16:10 atanisoft

OK, all fixed now, ready for a pull request once I test things.

RobertPHeller avatar Oct 26 '21 16:10 RobertPHeller

I think you mixed up the type of the template argument (PIN_NUM). Sometimes it is int, sometimes it is unsigned int.

balazsracz avatar Oct 26 '21 16:10 balazsracz

Yeah, a bit of a copy/paste snafu...

RobertPHeller avatar Oct 26 '21 16:10 RobertPHeller