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

[capture] Enable uJSON for SHA3 #248

Merged
merged 2 commits into from
Dec 11, 2023
Merged

Conversation

nasahlpa
Copy link
Member

@nasahlpa nasahlpa commented Dec 8, 2023

Similar to #218 , this PR enables uJSON support for capturing SHA3
traces. The device command handler code can be found in #20593.

@nasahlpa nasahlpa requested a review from m-temp December 8, 2023 11:04
@nasahlpa nasahlpa marked this pull request as ready for review December 8, 2023 11:04
Copy link
Collaborator

@m-temp m-temp left a comment

Choose a reason for hiding this comment

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

The ujson part looks good to me. With the CI I'm not familiar.
There a couple of time.sleep(0.01). I remember that there is (was ?) an issue that also deals with sleep commands, but I don't find it atm. Is this related and if so, should we track this to get rid of the sleeps in the future?

Comment on lines 436 to 441
raise Exception("Batch mode acknowledge error: Device and host not in sync")
return status
except Exception:
raise Exception("Batch mode acknowledge error: Device and host not in sync")
else:
raise Exception("Batch mode acknowledge error: Device and host not in sync")
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are two commands where _ujson_sha_sca_ack() is called: set_mask_off/on() and absorb_batch().
Thus, the error message might be missleading if this error happens in non-batch mode for the mask on/off function.
I think it is sufficient to just say "Acknowledge error:..." and use the exception information to track the function where it occurred.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I've changed the message.

Similar to lowRISC#218 , this PR enables uJSON support for capturing SHA3
traces. The device command handler code can be found in #20593.

Signed-off-by: Pascal Nasahl <[email protected]>
Signed-off-by: Pascal Nasahl <[email protected]>
@nasahlpa
Copy link
Member Author

Thanks for the review! I've created an issue (#256) to track the current problem with the necessary delay between uJSON commands.

@nasahlpa nasahlpa merged commit a30cf7d into lowRISC:master Dec 11, 2023
8 checks passed
@nasahlpa nasahlpa deleted the sha_ujson branch December 11, 2023 12:35
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