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

nrf_rpc: remove NULL pointer referenece #1545

Conversation

tomchy
Copy link
Contributor

@tomchy tomchy commented Oct 29, 2024

This reverts half of the changes introduced in the following PR:
#1493

Ref: NCSDK-29976

In case of error, the group pointer may be set to NULL.
This reverts part of changes introduced in the following PR:
nrfconnect#1493

Ref: NCSDK-29976

Signed-off-by: Tomasz Chyrowicz <[email protected]>
@alxelax
Copy link
Contributor

alxelax commented Oct 29, 2024

FYI @Damian-Nordic

Copy link
Contributor

@ahasztag ahasztag left a comment

Choose a reason for hiding this comment

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

This does not seem to be enough - the SUIT update still fails, although it passed after reverting the commits which introduced the change reverted here

@tomchy tomchy force-pushed the bugfix/nrf_rpc/NCSDK-29976_Fix_null_dereference branch from 8bb9f82 to 9c841b2 Compare October 29, 2024 14:03
@alxelax
Copy link
Contributor

alxelax commented Oct 29, 2024

This does not seem to be enough - the SUIT update still fails, although it passed after reverting the commits which introduced the change reverted here

Then it should be fixed correctly, not by reverting this commit. If changes are reverted, it breaks protocol serialization functionality.

Copy link
Contributor

@alxelax alxelax left a comment

Choose a reason for hiding this comment

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

The cause of issue should be investigated and fixed.

@tomchy tomchy force-pushed the bugfix/nrf_rpc/NCSDK-29976_Fix_null_dereference branch from 9c841b2 to 8bb9f82 Compare October 30, 2024 12:02
@tomchy
Copy link
Contributor Author

tomchy commented Oct 30, 2024

The cause of issue should be investigated and fixed.

Changed back to the partial revert. We are trying to fix the order of messages by adjusting the main thread priority.

@tomchy tomchy requested review from alxelax and ahasztag October 30, 2024 12:04
@alxelax
Copy link
Contributor

alxelax commented Oct 30, 2024

Hi @tomchy, please apply this patch: 47fdbee
This is more correct fix rather than partial reverting.

Or if you check this patch works, I'll create PR with this patch. Up to you.

@alxelax
Copy link
Contributor

alxelax commented Oct 30, 2024

Alternate PR with fix: #1548

@tomchy tomchy closed this Oct 31, 2024
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