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

lib_libvsprintf.c: add Add support for %pB parameter. #13536

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Otpvondoiats
Copy link

@Otpvondoiats Otpvondoiats commented Sep 19, 2024

Summary

  1. This is a new function that implements the method of printing the serialized data of the structure in a similar way to %pV %pS.
  2. Add LIBC_PRINT_EXTENSION option to control the opening of %p*, which is closed by default

Impact

None. This is a new parameter parsing and does not affect existing parameters.

Testing

pc: ubuntu 20.04,x86, SIM

void orb_info(FAR const char *format, FAR const char *name,
              FAR const void *data)
{
  struct lib_stdoutstream_s stdoutstream;
  struct va_format vaf;

  vaf.fmt = format;
  vaf.va  = (va_list *)data;

  lib_stdoutstream(&stdoutstream, stdout);
  lib_sprintf(&stdoutstream.common, "%s(now:%" PRIu64 "):%pB\n",
              name, orb_absolute_time(), &vaf);
}

order: uorb_listener -n 10 sensor_accel_uncal
out:

Mointor objects num:1
object_name:sensor_accel_uncal, object_instance:0
sensor_accel_uncal(now:57777592500):timestamp:57777591900,x:0.081402,y:0.689530,z:9.897630,temperature:31.253906
sensor_accel_uncal(now:57777632700):timestamp:57777632300,x:0.126893,y:0.792481,z:9.911995,temperature:31.253906
sensor_accel_uncal(now:57777672800):timestamp:57777672500,x:0.055066,y:0.708684,z:9.892841,temperature:31.253906
sensor_accel_uncal(now:57777713500):timestamp:57777713200,x:0.107739,y:0.682347,z:9.861717,temperature:31.253906
sensor_accel_uncal(now:57777754000):timestamp:57777753500,x:-0.040701,y:0.677559,z:9.751583,temperature:31.253906
sensor_accel_uncal(now:57777794200):timestamp:57777793800,x:0.117316,y:0.751779,z:9.797073,temperature:31.253906
sensor_accel_uncal(now:57777834600):timestamp:57777834100,x:0.081402,y:0.732626,z:9.734824,temperature:31.253906
sensor_accel_uncal(now:57777874700):timestamp:57777874400,x:0.062249,y:0.711078,z:9.732430,temperature:31.253906
sensor_accel_uncal(now:57777915000):timestamp:57777914700,x:0.114922,y:0.826000,z:9.742006,temperature:31.253906
sensor_accel_uncal(now:57777955600):timestamp:57777955000,x:0.098162,y:0.756568,z:9.809045,temperature:31.253906

timestamp:57777591900,x:0.081402,y:0.689530,z:9.897630,temperature:31.253906 is B parameter output results

@lupyuen
Copy link
Member

lupyuen commented Sep 19, 2024

[Experimental Bot, please feedback here]

No, this PR does not meet the NuttX requirements.

Missing Information:

  • Summary:
    • Lacks a clear explanation of why this change is necessary. Is it a bug fix? A feature request? Provide context.
    • "uorb info call" is too specific. What is the broader functional area impacted (e.g., logging, debugging, data formatting)?
  • Impact:
    • While you state "None," you need to address all impact points. Even if the answer is "NO," explicitly state it for clarity.
    • Your provided code snippet seems like it belongs in the "Testing" section.
  • Testing:
    • Insufficient detail:
      • You need to list the specific host operating systems, CPUs, compilers, target architectures, and boards you used for testing.
      • Provide actual testing logs, not just placeholders.
    • Unclear what is being tested: The code snippet is not self-explanatory. Describe the test case and expected results both before and after your change.

Recommendations:

  1. Expand the Summary: Explain the motivation for adding %pB. Is there a specific use case or issue it solves?
  2. Complete the Impact Section: Even if the impact is minimal, go through each point and confirm its status.
  3. Provide Detailed Testing Information:
    • List all relevant host and target configurations used.
    • Include complete testing logs demonstrating the issue before your change and the successful outcome after.
    • Clearly explain the purpose of your test code and the expected results.

By addressing these points, your PR will be more informative and likely to be reviewed and merged more quickly.

@yamt
Copy link
Contributor

yamt commented Sep 19, 2024

This is a new function that implements the method of printing the serialized data of the structure in a similar way to %pV %pS.

i was not aware of %pV and %pS.
their way to extend the functionality seems quite broken as it prevents us from providing standard %p.

please don't follow the mistake.

@github-actions github-actions bot added the Size: XS The size of the change in this PR is very small label Sep 19, 2024
@Otpvondoiats
Copy link
Author

image

@xiaoxiang781216
Copy link
Contributor

This is a new function that implements the method of printing the serialized data of the structure in a similar way to %pV %pS.

i was not aware of %pV and %pS. their way to extend the functionality seems quite broken as it prevents us from providing standard %p.

Linux kernel uses the similar approach to extend the format specifier:
https://www.[kernel.org/doc/Documentation/printk-formats.txt](https://www.kernel.org/doc/Documentation/printk-formats.txt)

please don't follow the mistake.

Could you give more info why it isn't good?

@yamt
Copy link
Contributor

yamt commented Sep 20, 2024

This is a new function that implements the method of printing the serialized data of the structure in a similar way to %pV %pS.

i was not aware of %pV and %pS. their way to extend the functionality seems quite broken as it prevents us from providing standard %p.

Linux kernel uses the similar approach to extend the format specifier: [https://www.kernel.org/doc/Documentation/printk-formats.txt](https://www.%5Bkernel.org/doc/Documentation/printk-formats.txt%5D(https://www.kernel.org/doc/Documentation/printk-formats.txt))

printk is a non-standard linux-internal api, which isn't meant to conform any standards.

please don't follow the mistake.

Could you give more info why it isn't good?

printf("%pS", NULL) should yield something like "0x0S".

besides that, these kinds of extensions are not recognized by the compiler's __format__ attribute.

@xiaoxiang781216
Copy link
Contributor

This is a new function that implements the method of printing the serialized data of the structure in a similar way to %pV %pS.

i was not aware of %pV and %pS. their way to extend the functionality seems quite broken as it prevents us from providing standard %p.

Linux kernel uses the similar approach to extend the format specifier: [https://www.kernel.org/doc/Documentation/printk-formats.txt](https://www.%5Bkernel.org/doc/Documentation/printk-formats.txt%5D(https://www.kernel.org/doc/Documentation/printk-formats.txt))

printk is a non-standard linux-internal api, which isn't meant to conform any standards.

please don't follow the mistake.

Could you give more info why it isn't good?

printf("%pS", NULL) should yield something like "0x0S".

besides that, these kinds of extensions are not recognized by the compiler's __format__ attribute.

Ok, @Otpvondoiats let's add an option in Kconfig to control this special format specifier.

@Otpvondoiats
Copy link
Author

This is a new function that implements the method of printing the serialized data of the structure in a similar way to %pV %pS.

i was not aware of %pV and %pS. their way to extend the functionality seems quite broken as it prevents us from providing standard %p.

Linux kernel uses the similar approach to extend the format specifier: [https://www.kernel.org/doc/Documentation/printk-formats.txt](https://www.%5Bkernel.org/doc/Documentation/printk-formats.txt%5D(https://www.kernel.org/doc/Documentation/printk-formats.txt))

printk is a non-standard linux-internal api, which isn't meant to conform any standards.

please don't follow the mistake.

Could you give more info why it isn't good?

printf("%pS", NULL) should yield something like "0x0S".
besides that, these kinds of extensions are not recognized by the compiler's __format__ attribute.

Ok, @Otpvondoiats let's add an option in Kconfig to control this special format specifier.

Get

@github-actions github-actions bot added Area: OS Components OS Components issues Size: S The size of the change in this PR is small and removed Size: XS The size of the change in this PR is very small labels Sep 20, 2024
@xiaoxiang781216
Copy link
Contributor

@yamt please review the new implementation which disable the extension by default.

@yamt
Copy link
Contributor

yamt commented Sep 21, 2024

@yamt please review the new implementation which disable the extension by default.

i feel it's difficult to use a kconfig which alters the behavior of very basic things like printf.
when you want to enable it, you need to audit every uses of printf family functions in your system.

isn't it simper to provide an alternative api, say, nuttx_printf, to provide the extension? (similarly to linux printk)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: OS Components OS Components issues Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants