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

Use C++ style callback interfaces as a variant of existing C function callbacks #404

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

peolsson
Copy link

Without breaking any compatibility, these changes extends tActisenseReader and tNMEA2000 classes to allow C++ interface style callbacks. This makes it easier to integrate with other C++ classes, instead of using simple C function pointers. This also fixes a bug in tActisenseReader that could end up in an endless loop.

If byte 'StartOfText' is received when an escape character was not
received previously, the method 'GetMessageFromStream' could end up in
an endless loop. This shouldn't happen when correct data is received,
but since it's data received on the UART, there are no guarantees that
the data is not corrupt.
Add an alternative callback method instead of using a C-style function
pointer when a new message is received. This callback is based on a
C++ interface which allows for handling a callback within a C++ class
context instead of a C function.
Add an alternative callback method instead of using a C-style function
pointer for various callbacks. This callback is based on a
C++ interface which allows for handling a callback within a C++ class
context instead of a C function.
@ttlappalainen
Copy link
Owner

Please check first void AttachMsgHandler(tMsgHandler *_MsgHandler); and tMsgHandler class. Not sure if I want third callback handler for tNMEA2000.

@peolsson
Copy link
Author

Please check first void AttachMsgHandler(tMsgHandler *_MsgHandler); and tMsgHandler class. Not sure if I want third callback handler for tNMEA2000.

Hi. Yes, I've seen that already, and that would solve the message callback handler, but it would not solve the callbacks for OnOpen and OnIsoRequest. Currently there is no (easy) way of handling these callbacks within a C++ class context, without declaring global variables etc.

@peolsson
Copy link
Author

Please let me know if there are any changes you want me to do, or if you want to close the pull-request again.

@ttlappalainen
Copy link
Owner

I am bit busy and had not yet time to think it carefully. But there is some clear problems.

  • It is not necessary to setup callback before open. It can be defined dynamically later. So OnOpen does not make sense in that case.
  • Class does not handle group function requests.

I understand your point, but have to think it more.

@peolsson
Copy link
Author

I am bit busy and had not yet time to think it carefully. But there is some clear problems.

  • It is not necessary to setup callback before open. It can be defined dynamically later. So OnOpen does not make sense in that case.
  • Class does not handle group function requests.

I understand your point, but have to think it more.

No worries - take your time :) Some comments on your thoughts:

  1. If you want to be able to handle OnOpen() callback within a C++ context, there is no way to do that today. Instead you would have to trigger a plain function, which wiould need to access the C++ object via a global variable or similar solution. I understand that the callback might be set after OnOpen was called, which would make that specific callback handler useless - however, if the user is not interested in handling OnOpen(), they could just leave the method body empty. We could also change the interface to implement (with empty body) the OnOpen callback instead of having it as a pure virtual.
  2. Group function requests are already handled with C++ class instances, so there was no need to handle them in another way. As already mentioned, the same applies for the message handler to some extent, so that part could be choosen to be removed if needed.

Regards,

Peter Olsson

@denravonska
Copy link
Contributor

denravonska commented Jun 14, 2024

It's been a while since I looked at this project but I would solve it in one of 3 ways depending on std and C compatibility requirements:

  • A callback with a context pointer (C compatibility)
  • A callback which is an std::function (can bind into methods, free functions and lambdas. Has some C++ standard requirements but nothing fancy)
  • A callback using fastdelegate or one of its incarnations (can bind into objects or free functions but is somewhat magic)

The cleanest would probably be std::function if it can be handled in such a way that the existing API isn't broken but rather wrapped when invoked in the "old" way.

@ttlappalainen
Copy link
Owner

At the time I started library I had to keep std:: away, since it did not compile for some MCUs. Not sure is it limitation anymore.

One problem is that change to core requires me to run tests for several different environments and MCU:s.

@peolsson
Copy link
Author

It's been a while since I looked at this project but I would solve it in one of 3 ways depending on std and C compatibility requirements:

  • A callback with a context pointer (C compatibility)
  • A callback which is an std::function (can bind into methods, free functions and lambdas. Has some C++ standard requirements but nothing fancy)
  • A callback using fastdelegate or one of its incarnations (can bind into objects or free functions but is somewhat magic)

The cleanest would probably be std::function if it can be handled in such a way that the existing API isn't broken but rather wrapped when invoked in the "old" way.

Yes, there are of course several ways to achieve similar results. The main reason for choosing a simple callback interface with an abstract class like I did, was to keep the implementation similar to the existing one, with low risk of compatibility issues (e.g. std::function requires C++11). I'm open to any suggestions though, so please let me know if you want me to consider any changes to this.

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

Successfully merging this pull request may close these issues.

3 participants