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 request message to CoAP response callback function #50

Merged
merged 10 commits into from
Jul 20, 2021

Conversation

Totalus
Copy link

@Totalus Totalus commented Apr 8, 2021

This pull request is a suggestion to add the CoAP request message as parameter to the CoAP response callback function.

// Original:
typedef CoAP_Result_t ( * CoAP_RespHandler_fn_t )(CoAP_Message_t* pRespMsg, NetEp_t* Sender);
// New definition:
typedef CoAP_Result_t ( * CoAP_RespHandler_fn_t )(CoAP_Message_t* pRespMsg, CoAP_Message_t* pReqMsg, NetEp_t* Sender);

Why is this useful ?

This allows to take better action on a transmission failures. By having access to the original message, we have access to the message ID and data. We can check the ID and data and if needed store back the message to memory to send it later and avoid any data loss. Currently when the transmission fails after the retries, the pRespMsg value is NULL so we don't have any info on the request it is related to and can't take any action.

WARNING

This change breaks the API, however it is not that big of a deal since it only requires you to add the pReqMsg parameter to all of your (probably very few) CoAP response callback function definitions.

@niondir
Copy link
Member

niondir commented Jul 20, 2021

@Totalus Thanks that looks great. I consider having the request message inside the handler is a good idea and implemented in most other frameworks as well.
Unfortunately it needs some rebase to solve conflicts before I can merge it. Would you be so kind to resolve the conflicts?

@niondir niondir merged commit 62508bb into lobaro:master Jul 20, 2021
@niondir
Copy link
Member

niondir commented Jul 20, 2021

All right, merging did work.

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