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

Add callback for CPX app messages #1279

Merged
merged 3 commits into from
May 24, 2023
Merged

Conversation

mfxjx000205
Copy link
Contributor

Dear Bitcraze:

In the PR #1273, my collaborator @RavenLite commit a PR about fixing the CPX_F_APP in CPX protocol. Now this work will be carried forward by me.

In this PR, we fix three main problems:

  1. We add a case about putting CPX_F_APP message into mixedQueue in cpx_internal_router.c to make sure that the CF can receive message with type CPX_F_APP.

  2. We add a handle in main code cpx.c. As @krichardsson said last time:"You need to add a function pointer and a function where an app can register a handler function for the pointer."But we believe that, as the name of this message category(CPX_F_APP) suggests, processing this message should be done in a user-defined app. So we added only an alert for the received message.

  3. We add a deepcopy function to carry the CPXpacket from cpxRx to user-defined cpx packet. We do this because we can't directly use the function named cpxInternalRouterReceiveOthers() in the app-layer due to the repeat reception. So we believe that a deep copy function is needed. Users can copy the packets received by the system to their own app without causing conflicts.

Thank you for your reviewing.

@krichardsson
Copy link
Contributor

krichardsson commented May 15, 2023

Thanks for your PR!

But we believe that, as the name of this message category(CPX_F_APP) suggests, processing this message should be done in a user-defined app. So we added only an alert for the received message.

I completely agree, and this is why I think the callback is what is needed.
I'm not sure I understand how your solution is supposed to work, how will the app be notified that data is available? This is what the callback would be used for.

I suggest something like this:

in cpx.h

typedef void (*cpxAppMessageHandlerCallback_t)(const CPXPacket_t* cpxRx);
void cpxRegisterAppMessageHandler(cpxAppMessageHandlerCallback_t callback);

and in cpx.c

void cpxRegisterAppMessageHandler(cpxAppMessageHandlerCallback_t callback) {
  appMessageHandlerCallback = callback;
}

// In the switch 

      case CPX_F_APP:
        if (appMessageHandlerCallback) {
          appMessageHandlerCallback(&cpxRx);
        }
        break;

This would enable an app to do something like:

void onCpxAppMessage(const CPXPacket_t* cpxRx) {
// Do stuff with the message, but quick to not block the router. Maybe a copy?
}

// Somewhere in the init code
cpxRegisterAppMessageHandler(onCpxAppMessage);

I don't think the deep copy function should be part of cpx.c, this is something an app can do if needed

What do you think?

@mfxjx000205 mfxjx000205 marked this pull request as draft May 15, 2023 12:36
@mfxjx000205
Copy link
Contributor Author

mfxjx000205 commented May 15, 2023

Dear @krichardsson:

Thank you for your help!

We misunderstood you before, because in our project we didn't consider using callback functions. so we set a deep copy function in the firmware to be able to read CPX packages in the user defined app without conflicts.

According to your help, we took your suggestion to optimize the code, and now the PR submission is already the optimized version. In this version:

1. Callback function is set in cpx.c.

2. CPX_F_APP message is put into mixedQueue in cpx_internal_router.c

Thanks again for your excellent advice, callback functions are not used much in our projects , but this is very important.

@mfxjx000205 mfxjx000205 marked this pull request as ready for review May 15, 2023 14:00
@krichardsson
Copy link
Contributor

Looking good, thanks for your contribution!
Merging

@krichardsson krichardsson merged commit 74d608d into bitcraze:master May 24, 2023
@krichardsson krichardsson changed the title Improvements to CPX base on PR#1273 Add callback for CPX app messages May 24, 2023
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.

2 participants