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

PIKSI 320 Add log message when state msg_id do not match request msg_id #539

Open
wants to merge 3 commits into
base: altti/before_Piksi_330
Choose a base branch
from

Conversation

jithk
Copy link

@jithk jithk commented Jan 11, 2023

@jithk jithk added the do not merge do not merge!!! label Jan 11, 2023
@jithk jithk requested a review from sokhealy January 11, 2023 23:20
@@ -303,7 +303,7 @@ static void setting_read_by_index_done_callback(uint16_t sender_id, uint8_t len,

/* Traverse the pending requests */
request_state_t *state = ctx->req_list;
while (state != NULL) {
while (state != NULL && state->msg_id == SBP_MSG_SETTINGS_READ_BY_INDEX_REQ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should leave the while loop alone and add an if (state->msg_id == SBP_MSG_SETTINGS_READ_BY_INDEX_REQ) for request_state_signal.

We still need the while loop to call state = state->next;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will consume all the remaining states. Is that expected?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not 100% sure. I do not know the original intention of this code block, but from the comments it seems like that is the intention, to /* Traverse the pending requests */.

@sokhealy sokhealy self-requested a review January 12, 2023 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge do not merge!!!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants