Added a C wrapper

[ma-ludw]

Added a wrapper to use with c code.
To use the enums in the c code, I added theme additionally in the c header. The enums are extended with a SIZE value to check that they have in the c header and the c++ header the same size.

[pjueon]

Thanks for the contribution.
Do you have any idea for implementing C wrapper for PWM class?

[pjueon]

Also, if the wrapper supports GPIO_NUMBERING_MODES_TEGRA_SOC and GPIO_NUMBERING_MODES_CVM,
the channel types in the API should be const char* instead of int because these numbering modes use channel name(string).

Or maybe you could just remove these numbering modes for the C wrapper.

[ma-ludw]

Theoretically, it should be possible to have a wrapper around the PWM class. I can do it when I have time.

The problem with const char* and int is, in C there is no function overloading. So either you have two functions with, for example, a different suffix like _char and _int, or there is only one of the options. It would be possible to use only char arrays and leaf the int away.

[pjueon]

Comment for SIZE enum and check_enum_equality function:

check_enum_equality only works properly when the number of options is changed. It would not work if the order or the name of options is changed. For example:

    // Pin Numbering Modes
    enum class NumberingModes
    {
        // Changed the order of modes
        None,
        CVM,     
        TEGRA_SOC,
        BCM,
        BOARD,
        SIZE // the size is not changed
    };

And I think adding SIZE at the end of the enums in the public API just for checking if they are equal to c header is not very straightforward approach. More straightforward approach would be like this:

    // check if enum size of c wrapper and the cpp code has the same length at compile time
    __attribute__((unused)) void check_enum_equality()
    {
        static_assert(static_cast<GPIONumberingModes>(GPIO::NumberingModes::BOARD) == GPIO_NUMBERING_MODES_BOARD,
                      "c wrapper enum  must be equal to c++ enum");

        static_assert(static_cast<GPIONumberingModes>(GPIO::NumberingModes::BCM) == GPIO_NUMBERING_MODES_BCM,
                      "c wrapper enum  must be equal to c++ enum");

        static_assert(static_cast<GPIONumberingModes>(GPIO::NumberingModes::TEGRA_SOC) == GPIO_NUMBERING_MODES_TEGRA_SOC,
                      "c wrapper enum  must be equal to c++ enum");

        // add more static_assert ...
    }

With this approach, you don't need to change the public API and check_enum_equality function will work properly even if the order of the options are changed.

Or you can make enum converter functions and remove check_enum_equality function:

GPIONumberingModes ToCStyleEnum(GPIO::NumberingModes mode)
{
    switch (mode)
    {
    case GPIO::NumberingModes::BOARD:
        return GPIO_NUMBERING_MODES_BOARD;
    case GPIO::NumberingModes::BCM:
        return GPIO_NUMBERING_MODES_BCM;
    case GPIO::NumberingModes::TEGRA_SOC:
        return GPIO_NUMBERING_MODES_TEGRA_SOC;
    case GPIO::NumberingModes::CVM:
        return GPIO_NUMBERING_MODES_CVM;
    case GPIO::NumberingModes::None:
        return GPIO_NUMBERING_MODES_NONE;
    default:
        throw std::runtime_error("invalid numbering mode");
    }
} 

// ...

GPIONumberingModes gpio_getmode() { return ToCStyleEnum(GPIO::getmode()); }

[pjueon]

Theoretically, it should be possible to have a wrapper around the PWM class. I can do it when I have time.

Great. I'll look forward to it.

The problem with const char* and int is, in C there is no function overloading. So either you have two functions with, for example, a different suffix like _char and _int, or there is only one of the options. It would be possible to use only char arrays and leaf the int away.

Hmm.. I prefer the later one (using only const char* as channel).

[ma-ludw]

check_enum_equality only works properly when the number of options is changed. It would not work if the order or the name of options is changed.

This is true. The problem is, if you only check the equality of the single enum values, you don't check if in the number of elements is equal. When you for example add a new one to the c++ header, it would then not be checked if you had added it in the c header.

Or you can make enum converter functions and remove check_enum_equality function:

This does not enforce the developer to always update both (c and c++) enum structs. So it would be possible to update only one of both.

This is the benefit of adding a SIZE value to the enum class. So if you add a value, it is checked if the number of the values in the c and c++ header are equal.

[pjueon]

Ok. I thought that your C code will work properly even if the new option was added only to the C++ headers,
because the C header doesn't have the the new option so the user cannot pass it to your C code.

But now that I realized that there would be problems when your C code uses the enums as output.

I'm still not a fan of adding SIZE to the enum classes. So I came up with another trick using template and macro.

I added GPIO::details::enum_size to master branch. You can get the size of enums without adding SIZE at the end of the enums.

    // check if enum size of c wrapper and the cpp code has the same length at compile time
    __attribute__((unused)) void check_enum_equality()
    {
#define CHECK_ENUM_EQUALITY_MSG "c wrapper enum must be equal to c++ enum"

        static_assert(static_cast<GPIONumberingModes>(GPIO::details::enum_size<GPIO::NumberingModes>::value) ==
                          GPIO_NUMBERING_MODES_SIZE,
                      CHECK_ENUM_EQUALITY_MSG);

        static_assert(static_cast<GPIODirections>(GPIO::details::enum_size<GPIO::Directions>::value) ==
                          GPIO_DIRECTIONS_SIZE,
                      CHECK_ENUM_EQUALITY_MSG);

        static_assert(static_cast<GPIOEdge>(GPIO::details::enum_size<GPIO::Edge>::value) == GPIO_EDGE_SIZE,
                      CHECK_ENUM_EQUALITY_MSG);

#undef CHECK_ENUM_EQUALITY_MSG
    }

[pjueon]

Thanks for the update.

Here's the list that I think we need to do next:

• Update the CMakeList.txt to install the c_wrapper header file.
  • OR move the c_wrapper to samples directory and remove it from the public API.
    Then users could use it as an example and implement the wrapper by themselves for their needs.
• Add a cmake option for c_wrapper (maybe not necessary)
• Update the README
• Implement a wrapper for PWM class (maybe not necessary for this PR)

@ma-ludw
Let me know your opinion.

[ma-ludw]

I would prefer it in the public API and not as an example.
It is nicer if you use the project as a git submodule. Otherwise, the user has to maintain the wrapper himself.

I'm not familiar with c_make options, so the decision is up to you.

I'will update the README and optionally add a wrapper for the PWM class.
Unfortunately, I don't have time in the next two weeks. I will hopefully find time to implement these points afterwards.

[pjueon]

An idea for the implementing the PWM class:

// The names can be changed...
typedef int gpio_pwm;

gpio_pwm gpio_create_pwm(int channel, int frequency_hz);
bool gpio_destroy_pwm(gpio_pwm handle);

void gpio_start_pwm(gpio_pwm handle, double duty_cycle_percent);
void gpio_stop_pwm(gpio_pwm handle);
void gpio_change_pwm_frequency(gpio_pwm handle, int frequency_hz);
void gpio_change_pwm_duty_cycle(gpio_pwm handle, double duty_cycle_percent);

// make a resource pool for the PWM objects for the implementation...

@ma-ludw