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

samples: zbus: add leading zeros while printing duration #60076

Merged
merged 1 commit into from
Aug 18, 2023

Conversation

jilaypandya
Copy link
Contributor

This change will print leading zeros in the event of duration being less that 0.1 nano-secs

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hello @jilaypandya, and thank you very much for your first pull request to the Zephyr project!

A project maintainer just triggered our CI pipeline to run it against your PR and ensure it's compliant and doesn't cause any issues. You might want to take this opportunity to review the project's Contributor Expectations and make any updates to your pull request if necessary. 😊

@jilaypandya jilaypandya force-pushed the fix_zbus_benchmark branch 2 times, most recently from 7fa0a9e to ef1930a Compare July 6, 2023 09:09
@jilaypandya jilaypandya changed the title zbus: samples: benchmark: add leading zeros while printing duration samples: zbus: benchmark: add leading zeros while printing duration Jul 6, 2023
Copy link
Contributor

@rodrigopex rodrigopex left a comment

Choose a reason for hiding this comment

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

Hi @jilaypandya, thank you for the contribution. Could you point out an example where it is necessary? At first sight, I cannot see the actual benefit of adding so many leading zeros on that. Which architecture is returning that small latency? Which scenario (message size and number of consumers)?

I would suggest you follow the commit message guidelines. I suppose the benchmark: is not necessary on the PR and commit title because it is not a Zephyr area it is a sample in this case. It is important to point out the following aspects of the change in the commit message body:

  • what the change does,
  • why you chose that approach,
  • what assumptions were made, and
  • how you know it works – for example, which tests you ran.

Another tip: Sometimes, it is better to open an issue first. After some discussion, the PR is the result. Ok?

@rodrigopex rodrigopex added Enhancement Changes/Updates/Additions to existing features priority: low Low impact/importance bug labels Jul 6, 2023
@jilaypandya
Copy link
Contributor Author

jilaypandya commented Jul 6, 2023

Hi @jilaypandya, thank you for the contribution. Could you point out an example where it is necessary? At first sight, I cannot see the actual benefit of adding so many leading zeros on that. Which architecture is returning that small latency? Which scenario (message size and number of consumers)?

Hi @rodrigopex,

Thanks for pointing out the missing stuff. I basically took the benchmark example and ran it on mimxrt1060_evk and got the following results. While evaluating internally when we reduced the payload, we were able to transfer the messages within few microseconds, that's how I stumbled upon this bug, that the duration is not getting shown correctly.

*** Booting Zephyr OS build zephyr-v3.4.0-591-g011a55d5a1fd ***
I: Benchmark 1 to 8: Dynamic memory, SYNC transmission and message size 256
I: Bytes sent = 262144, received = 262144
I: Average data rate: 564.52MB/s
I: Duration: 0.000442848s

@442848

Before the change it looks like this

*** Booting Zephyr OS build zephyr-v3.4.0-591-geb58a77a7ad0 ***
I: Benchmark 1 to 8: Dynamic memory, SYNC transmission and message size 256
I: Bytes sent = 262144, received = 262144
I: Average data rate: 564.61MB/s
I: Duration: 0.442782s

@442782

@jilaypandya jilaypandya changed the title samples: zbus: benchmark: add leading zeros while printing duration samples: zbus: add leading zeros while printing duration Jul 6, 2023
@jilaypandya jilaypandya force-pushed the fix_zbus_benchmark branch 4 times, most recently from b3cdca7 to 3392ceb Compare July 6, 2023 14:17
@rodrigopex
Copy link
Contributor

@jilaypandya, you are right. There is a bug there. Let's go ahead with your submission. It needs to be fixed. Try running ./scripts/checkpatch/maintainer-checkpatch.bash before pushing to verify that locally.

Suggestion to the commit message:

samples: zbus: fix benchmark duration output 

The benchmark duration is in nanoseconds, and it logs the duration to the console in seconds.
The conversion is wrong for scenarios where the execution lasts less than 100 milliseconds. 
Fix that by adding leading zeros while printing duration.

Signed-off-by: Jilay Pandya <jilay.pandya@zeiss.com>

The lines must have up to 75 characters. Double-check that if you are going to use some of the suggestions.

@rodrigopex rodrigopex added bug The issue is a bug, or the PR is fixing a bug and removed Enhancement Changes/Updates/Additions to existing features labels Jul 6, 2023
Copy link
Contributor

@rodrigopex rodrigopex left a comment

Choose a reason for hiding this comment

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

I would like to ask you to adjust the commit message to meet the standard. Please perform the following changes:

  1. Add a line between the title and the commit message body;
  2. Try to keep the message body line length as close as possible to 75, but never more than that. They look weird when they have much less than that.

Thank you.

The benchmark duration is in nanoseconds, and it logs the duration to
the console in seconds. The conversion is wrong for scenarios where
the execution lasts less than 100 milliseconds. Fix that by adding
leading zeros while printing duration.

Signed-off-by: Jilay Pandya <jilay.pandya@zeiss.com>
@carlescufi carlescufi merged commit 7a9cda9 into zephyrproject-rtos:main Aug 18, 2023
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hi @jilaypandya!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

@jilaypandya jilaypandya deleted the fix_zbus_benchmark branch September 11, 2023 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Zbus bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants