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

[transport] generate log message on crc error #114

Merged
merged 1 commit into from
Feb 14, 2023

Conversation

r00t-
Copy link
Collaborator

@r00t- r00t- commented Feb 9, 2023

my version of #109

@r00t- r00t- mentioned this pull request Feb 9, 2023
@r00t-
Copy link
Collaborator Author

r00t- commented Feb 9, 2023

@jahir @mbehr1 care to review before merge?

Copy link

@mbehr1 mbehr1 left a comment

Choose a reason for hiding this comment

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

lgtm

goto error;
*msg->crc != sml_crc16kermit_calculate(&(buf->buffer[msg_start]), len)
){
fprintf(stderr, "libsml: sml_message_parse(): crc mismatch, dropping message\n");
Copy link

Choose a reason for hiding this comment

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

the len of the message might be useful for debugging

Copy link
Collaborator Author

@r00t- r00t- Feb 14, 2023

Choose a reason for hiding this comment

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

a hexdump would be even more useful, but we actually shouldn't be writing to stderr at all... :\

Copy link
Member

@jahir jahir left a comment

Choose a reason for hiding this comment

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

I agree with @mbehr1, it's often helpful to get some more information in case of errors.

Besides that: lgtm, too

@r00t- r00t- merged commit 64c5e2c into volkszaehler:master Feb 14, 2023
@r00t-
Copy link
Collaborator Author

r00t- commented Feb 14, 2023

thanks.

i don't think it makes much sense to add much more diagnostics until we have a better way of reporting/logging errors.
(when we should probably give the application the option to retrieve complete raw data for storing/debugging.)

but this is an improvement for now, and i think #103 already covers the topic.

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