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

Allow use of C++ semantics #2258

Open
PhilippMolitor opened this issue Sep 1, 2023 · 10 comments
Open

Allow use of C++ semantics #2258

PhilippMolitor opened this issue Sep 1, 2023 · 10 comments
Milestone

Comments

@PhilippMolitor
Copy link

PhilippMolitor commented Sep 1, 2023

Background: #2252

Current Status

The U8g2 library is based around a C core library, that implements the actual functionality of U8g2 in object-oriented C.
Additionally, the library provides a thin C++ wrapper around that C core library for convenience.

Some of the C++ semantics do not work with this C++ layer though.
Consider this example, which showcases what works and what doesn't.

uint8_t cb_test_a(mui_t* ui, uint8_t msg) {
  // ...
}

class UIManager {
 private:
  // LCD
  U8G2Class m_lcd = U8G2Class(U8G2_R0);
  MUIU8G2 m_menuConfig;

  fds_t m_menuContent[256] =
      // clang-format off
      MUI_FORM(0)
      // ... more U8g2 MUI stuff
      // clang-format on
      ;
  muif_t m_menuData[256] = {
      // A callback function just like from the manual. Works!
      MUIF_RO("TA", cb_test_a),

      // A lambda function, no scoped variables. Works!
      MUIF_RO("TB",
              [](mui_t* ui, uint8_t msg) {
                // do something...
                return (uint8_t)0;
              }),

      // A class method. Does not work!
      MUIF_RO("TC", m_menuConfig.leaveForm),

      // A lambda function, passes `this` as reference. Does not work!
      MUIF_RO("TD",
              [&](mui_t* ui, uint8_t msg) {
                this->m_menuConfigRedraw = true;
                return (uint8_t)0;
              }),
      //
  };

  // state
  bool m_menuConfigRedraw = true;

 public:
  void begin() {
    // LCD init
    m_lcd.begin();
    m_menuConfig.begin(m_lcd, m_menuContent, m_menuData,
                       sizeof(m_menuData) / sizeof(muif_t));
    m_menuConfig.gotoForm(1, 0);
  }

  void updateDisplayMenu() {
    if (!m_menuConfigRedraw)
      return;

    m_lcd.firstPage();
    do {
      m_lcd.setFont(u8g2_font_helvR08_tr);
      m_menuConfig.draw();
    } while (m_lcd.nextPage());

    m_menuConfigRedraw = false;
  }

  void updateDisplayIdleScreen() {
    m_lcd.firstPage();
    do {
      m_lcd.clearBuffer();
      m_lcd.drawRFrame(2, 2, 124, 60, 2);

      m_lcd.setFont(u8g2_font_helvR08_tr);
      m_lcd.setCursor(20, 50);
      m_lcd.print(millis());

    } while (m_lcd.nextPage());
  }

  void updateDisplay() {
    if (m_menuConfig.isFormActive()) {
      updateDisplayMenu();
    } else {
      updateDisplayIdleScreen();
    }
  }

Ideas for solving the issue

The current implementation defines the call back with:

typedef uint8_t (*muif_cb)(mui_t *ui, uint8_t msg);

You suggested to use a friend function, which I think is not very elegant for a few reasons:

  • Multiple instances of the UI Class. I don't think there is a nice way to handle multiple instances at the same time with friend functions, or it might make the code hard to read.
  • The friend function would be implemented somewhere else in the source, making the code again harder to understand.
  • If I simply want to read a member variable of UIManager, there is no quick way to do that.
  • There possibly is a better solution which allows for more flexible & concise code:

A modern C++ approach probably would choose the std::function type:

#include <functional>

typedef std::function<uint8_t(mui_t *ui, uint8_t msg)> muif_cb;

A good practical example of that is Espressif's implementation of attachInterrupt() in their Arduino Core:
https://github.com/espressif/arduino-esp32/blob/master/cores/esp32/FunctionalInterrupt.h

@olikraus olikraus added this to the 2.35 milestone Sep 1, 2023
@olikraus
Copy link
Owner

olikraus commented Sep 1, 2023

I don't understand how your suggestion with std::function can solve the class member and this arg example.
I mean, I am sure that also the Espressif attachIterrupt does not allow to use class member functions, or did I miss something here.

@PhilippMolitor
Copy link
Author

For me, using a lambda function works fine in this example:

attachInterrupt(somePin, [&]() { someMemberFunction(); }, CHANGE);

But there also seems to be another solution suggested by maxgerhardt:

attachInterrupt(somePin, std::bind(&MyClassName::someMemberFunction, this), CHANGE);

(see https://community.platformio.org/t/how-to-assign-a-class-method-as-an-interrupt-handler/20501/2)

@olikraus
Copy link
Owner

olikraus commented Sep 2, 2023

The "bind" approach seems to be promising, but you need a valid "this" pointer while building the MUIF array.

https://docs.w3cub.com/cpp/utility/functional/bind

@PhilippMolitor
Copy link
Author

PhilippMolitor commented Sep 2, 2023

I added the MUIF array to the member variables, just like in the example. Doesn't seem like a bad thing to me. In this scope, it should have access to the this operator (or is this just the case for member functions?). Personally, I prefer the lambda function approach for readability (and maybe executing stuff there that should not be executed by a member function), but I guess that is just a personal style perference.

@PhilippMolitor
Copy link
Author

PhilippMolitor commented Sep 3, 2023

I am currently testing the implementation of std::function.

This is what i changed in mui.h:

#include <functional>
typedef std::function<uint8_t(mui_t* ui, uint8_t msg)> muif_cb;
// typedef uint8_t (*muif_cb)(mui_t* ui, uint8_t msg);

Although my project code now accepts a lambda function with a reference to this, the compiler does not like the #include <functional>, as this std libary is not available in the AVR toolchain.

As I am compiling for an ESP32 with the PlatformIO system, it seems like the U8g2 library itself chooses that compiler. I have to investigate further to make this run and then test if my changes work on the running MCU.

If #include <functional> is no option, maybe we could borrow an implementation from the embedded template library:
https://github.com/ETLCPP/etl/blob/master/include/etl/function.h

@olikraus olikraus modified the milestones: 2.35, Future Dec 2, 2023
@vortigont
Copy link

not being able to use functional callbacks or class members is a bit frustrating :(
As alternative solution it is possible to use additional void* arg parameter in callback function type. Then it could be used to pass a pointer to this and typecasting it to required class pointer. For example this way it is used in RTOS C callback prototypes which allows to bind tasks, timers, etc with a member functions of c++ class instances. It's highly compatible and stays within C. But this would require changing types for all of mui_u8g2_*'s, plus whole a lot of macros. Plus expand struct muif_struct, struct mui_u8g2_list_struct with void* arg member. Plus it's a braking change for existing code... ugh...

// mui.h
typedef uint8_t (*muif_cb)(mui_t *ui, uint8_t msg, void* arg);

// then somewhere in the class
class MyClass {
  uint8_t myvar;
  void dosomething(uint8_t msg);

  muif_t muif_main_configuration_menu_list[16] = {
    // ...
    MUIF_RO("HR", [](mui_t* ui, uint8_t msg, void* self){ static_cast<MyClass*>(self)->dosomething(msg); }, this),
};

  fds_t mymenu[10] =
      // A non-capturing lambda function 
      MUIF_RO("AB",
              [](mui_t* ui, uint8_t msg, void* self) {
                static_cast<MyClass*>(self)->myvar = 42;
                return (uint8_t)0;
              }),
    // in case of static function this arg must be NULL'ed
    MUIF_RO("HR", mui_hrule, NULL),                       // draw horizontal line on display
  };

@olikraus
Copy link
Owner

Well, yes "mui" is truly minimal. I wanted something small... I am sure there are better UI libs out there ;-)

@vortigont
Copy link

to my surprise most of them lack functional callbacks either :) Your one is really nice to have bundled with main u8g2 lib.
Maybe there could be a compromise solution? For example having typedef std::function for callback function at least for those architectures where it is available. Some #ifdef's would not be much of a problem?

@olikraus
Copy link
Owner

Any solution should allow standard C compiler so handle mui code. But I am happy to included PRs if this precondition is met.

@vortigont
Copy link

I scratched my head for weeks trying to work-around it and somehow pass a reference to this* into MUI's callback struct so not to bring std::function dependency. But one way or the the other it has to change the structs with additional void* pointers and function arguments. So I guess it would not be a feasible option to keep backward compatibility with all the platforms u8g2 supports. MUI is pretty compact and works just fine for it's purpose.
So I gave up the idea and re-implemented MUI-like lib for C++ using pure u8g2's back-end. Looks bulky but allows full dynamic menu elements with func callbacks and capturing lambda's and integrates nicely into user class instances. It's just proof of concept, but works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants