ulp_riscv_print: Add convenience function that supports different widths (IDFGH-12124) #13180

mickeyl · 2024-02-14T16:19:25Z

ULP's print_hex is pretty handy, but hardcoded to uint32 with 8 digits. This patch adds a convenience function that supports different widths.

CLAassistant · 2024-02-14T16:19:31Z

All committers have signed the CLA.

github-actions · 2024-02-14T16:20:07Z

	Messages
📖	🎉 Good Job! All checks are passing!

👋 Hello mickeyl, we appreciate your contribution to this project!

📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more.

🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project.

Click to see more instructions ...

This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...

We do welcome contributions in the form of bug reports, feature requests and pull requests via this public GitHub repository.

This GitHub project is public mirror of our internal git repository

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved, we synchronize it into our internal git repository.
4. In the internal git repository we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
5. If the change is approved and passes the tests it is merged into the default branch.
5. On next sync from the internal git repository merged change will appear in this public GitHub repository.

Generated by 🚫 dangerJS against 89413ea

sudeep-mohanty · 2024-02-15T08:04:25Z

components/ulp/ulp_riscv/ulp_core/include/ulp_riscv_print.h

@@ -37,6 +37,7 @@ void ulp_riscv_print_str(const char *str);
 * @param Hex number to print
 */
 void ulp_riscv_print_hex(int h);
+void ulp_riscv_print_hex_with_number_of_digits(int h, int number_of_digits);


Could you also add a function description to explain the purpose and the arguments?

sudeep-mohanty · 2024-02-15T08:05:31Z

components/ulp/ulp_riscv/ulp_core/ulp_riscv_print.c

@@ -49,3 +49,24 @@ void ulp_riscv_print_hex(int h)
        h <<= 4; // move the 2nd leftmost byte to the left, to be extracted next
    }
 }
+
+void ulp_riscv_print_hex_with_number_of_digits(int h, int number_of_digits)


Since the input here is int h, the maximum number of hex digits would be 8. In this case, I assume the purpose of this function would be to print < 8 bytes. Is the understanding correct?

sudeep-mohanty · 2024-02-15T08:06:36Z

components/ulp/ulp_riscv/ulp_core/ulp_riscv_print.c

+{
+    int x;
+    int c;
+


Would be good to add a sanity check for number_of_digits < 0 and number_of_digits >= 8. The first case should result in a error for the user and the second case should default the call to ulp_riscv_print_hex().

sudeep-mohanty · 2024-02-15T08:09:12Z

components/ulp/ulp_riscv/ulp_core/ulp_riscv_print.c

+        return;
+    }
+
+    // Does not print '0x', only the digits (8 digits to print)


Suggested change

// Does not print '0x', only the digits (8 digits to print)

// Does not print '0x', only the digits specified by the number_of_digits argument

sudeep-mohanty · 2024-02-15T08:10:15Z

Thanks for the contribution @mickeyl! I've left some minor queries and comments. Once they are addressed, we should be good to merge. Thanks!

mickeyl · 2024-03-17T12:37:34Z

@sudeep-mohanty Thanks for your comments. I have rebuild the pull request putting them into account. Please have another look.

sudeep-mohanty · 2024-03-18T11:19:32Z

Hi @mickeyl, I do not see the updated changes. Maybe you missed committing them? 🤔

mickeyl · 2024-03-18T12:03:40Z

@sudeep-mohanty Indeed, my force-push didn't work for some reason. It should be visible now.

sudeep-mohanty · 2024-03-18T12:40:37Z

Thanks @mickeyl. Changes LGTM. I shall do some more tests and then merge it.

sudeep-mohanty · 2024-03-18T13:03:21Z

sha=89413eaab6501749f54072d80065c499b72c2c26

espressif-bot added the Status: Opened Issue is new label Feb 14, 2024

github-actions bot changed the title ~~ulp_riscv_print: Add convenience function that supports different widths~~ ulp_riscv_print: Add convenience function that supports different widths (IDFGH-12124) Feb 14, 2024

espressif-bot assigned sudeep-mohanty Feb 15, 2024

sudeep-mohanty reviewed Feb 15, 2024

View reviewed changes

espressif-bot added Status: In Progress Work is in progress and removed Status: Opened Issue is new labels Feb 15, 2024

mickeyl force-pushed the master branch from d8d54bf to 4042a48 Compare March 17, 2024 12:36

feat(ulp): Add convenience print function that supports different widths

89413ea

mickeyl force-pushed the master branch from 4042a48 to 89413ea Compare March 18, 2024 12:01

sudeep-mohanty added the PR-Sync-Merge Pull request sync as merge commit label Mar 18, 2024

sudeep-mohanty self-requested a review March 18, 2024 13:16

sudeep-mohanty approved these changes Mar 18, 2024

View reviewed changes

espressif-bot added Status: Done Issue is done internally Resolution: NA Issue resolution is unavailable and removed Status: In Progress Work is in progress labels Mar 19, 2024

espressif-bot closed this in 08dead4 Mar 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ulp_riscv_print: Add convenience function that supports different widths (IDFGH-12124) #13180

ulp_riscv_print: Add convenience function that supports different widths (IDFGH-12124) #13180

mickeyl commented Feb 14, 2024

CLAassistant commented Feb 14, 2024 •

edited

github-actions bot commented Feb 14, 2024 •

edited

sudeep-mohanty Feb 15, 2024

sudeep-mohanty Feb 15, 2024

sudeep-mohanty Feb 15, 2024 •

edited

sudeep-mohanty Feb 15, 2024

sudeep-mohanty commented Feb 15, 2024

mickeyl commented Mar 17, 2024

sudeep-mohanty commented Mar 18, 2024

mickeyl commented Mar 18, 2024

sudeep-mohanty commented Mar 18, 2024

sudeep-mohanty commented Mar 18, 2024

	// Does not print '0x', only the digits (8 digits to print)
	// Does not print '0x', only the digits specified by the number_of_digits argument

ulp_riscv_print: Add convenience function that supports different widths (IDFGH-12124) #13180

ulp_riscv_print: Add convenience function that supports different widths (IDFGH-12124) #13180

Conversation

mickeyl commented Feb 14, 2024

CLAassistant commented Feb 14, 2024 • edited

github-actions bot commented Feb 14, 2024 • edited

sudeep-mohanty Feb 15, 2024

Choose a reason for hiding this comment

sudeep-mohanty Feb 15, 2024

Choose a reason for hiding this comment

sudeep-mohanty Feb 15, 2024 • edited

Choose a reason for hiding this comment

sudeep-mohanty Feb 15, 2024

Choose a reason for hiding this comment

sudeep-mohanty commented Feb 15, 2024

mickeyl commented Mar 17, 2024

sudeep-mohanty commented Mar 18, 2024

mickeyl commented Mar 18, 2024

sudeep-mohanty commented Mar 18, 2024

sudeep-mohanty commented Mar 18, 2024

CLAassistant commented Feb 14, 2024 •

edited

github-actions bot commented Feb 14, 2024 •

edited

sudeep-mohanty Feb 15, 2024 •

edited