bakerstu / openmrn

OpenMRN (Open Model Railroad Network)
BSD 2-Clause "Simplified" License
55 stars 28 forks source link

MultiConfigurePC not usable? #582

Open RobertPHeller opened 2 years ago

RobertPHeller commented 2 years ago

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.

atanisoft commented 2 years ago

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.

RobertPHeller commented 2 years ago

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?

atanisoft commented 2 years ago

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

RobertPHeller commented 2 years ago

OK, so GpioWrapper is effectively depreciated?

atanisoft commented 2 years ago

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

balazsracz commented 2 years ago

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.

atanisoft commented 2 years ago

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!)

balazsracz commented 2 years ago

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

atanisoft commented 2 years ago

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

RobertPHeller commented 2 years ago

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 LinuxGpio::instance_’ const LinuxGpio LinuxGpio::instance_; ^~~~~

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

atanisoft commented 2 years ago

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 commented 2 years ago

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

RobertPHeller commented 2 years ago

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

balazsracz commented 2 years ago

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

RobertPHeller commented 2 years ago

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