Revert "shell: support floating point output..."

[andrewboie]

This reverts commit e812ee6.

This is not the right approach. If the shell is incapable of
using standard libc printing functions, it needs to use (and
possibly extend) the existing printk() code already in this
library. See #29202

Signed-off-by: Andrew Boie [email protected]

[pabigot]

As I tried to argue in #29171 I think this is the wrong path forward.

z_prf() provides functionality that is crucial to consistent and complete formatting support in the shell, and possibly the logger, and possibly tracing, all of which invoke it under various conditions. Stuffing it back into minimal libc doesn't help anybody who needs this functionality with other libc solutions.

However, it's been made very clear that the owner of lib/os doesn't want it there, nor to use it to make printk() functionally equivalent.

Perhaps we need another place to put utilities that are generally useful to the entire OS.

[nashif]

However, it's been made very clear that the owner of lib/os doesn't want it there, nor to use it to make printk() functionally equivalent.

Why are making this discussion about ownership?

This is still a PR where you can object and raise concerns and if you have a better solution and think we should not revert, please make the case and others are welcome to do the same.
The change being reverted went in without enough review from all parties concerned which is unfortunate and this can happen. Reverting and looking for better solutions should always be an option.

[pabigot]

The change being reverted went in without enough review from all parties concerned which is unfortunate

Don't know what to say about that: #27655 was open for two weeks with the lib/os codeowners added as reviewers when it was created.

Why are making this discussion about ownership?

It's in lib/os:

That is owned by me and Andy. This isn't up for discussion.

Since lib/os has things like base64 and json encoding conversion it seemed reasonable to be there.

But if it the problem is it needs to go somewhere else, and/or be named something else, I can certainly do that. Who will own that thing, and what do they want it to be called? Because making proposals that are immediately rejected is not effective use of anybody's time.

If the problem is #29202, moving it back to minimal libc (or anywhere else) isn't going to change much.

[nashif]

that is owned by me and Andy. This isn't up for discussion.

Since lib/os has things like base64 and json encoding conversion it seemed reasonable to be there.

The above refers to making decisions in a meeting. My guess is that the nature and purpose of bringing this into a meeting is not well understood because not everyone can join such meetings.

But if it the problem is it needs to go somewhere else, and/or be named something else, I can certainly do that.

I think it is not about lib/os or lib/. this is about moving it from libc and making it part of the OS also in the case where you use another libc, basically duplicating the code. So we need to step back and figure why shell, logging and others are using this first place and how we can solve the problem at the core.

[pabigot]

So we need to step back and figure why shell, logging and others are using this first place and how we can solve the problem at the core.

Agreed. I think the rationale is pretty well described at #27655:

The shell requires a *printf function that produces its output by calling a function like putchar(3c) to output one character at a time. This eliminates dynamic or static allocation of buffers large enough to hold arbitrary messages.

When CONFIG_MINIMAL_LIBC=y a custom implementation z_prf() is available which is capable of producing float values. It's used to implement s*printf() in minimal libc, and is part of the minimal libc library.

When CONFIG_NEWLIB_LIBC=y the minimal libc library is not available, so z_prf() is not available, so Zephyr falls back to z_vprintk() which also generates output through a putchar()-like function, but is not capable of printing floating point values.

Shell and logging require a way to generate formatted text output that doesn't require arbitrary-sized buffers, because they output to a variety of endpoints (serial, network, debugger back channel). A good solution is the API for z_prf() and z_vprintk():

typedef int (*out_function)(void *ctx);
int fmt_function(out_function out, void *ctx, const char *fmt, va_args args);

There are no such functions in C libraries; certainly not in newlib.

Zephyr applications need to support generating 64-bit integers and floating point in formatted output, certainly from the shell and arguably from logging.

It's certainly technically possible to take z_prf() or vprintk(), rename it, and attempt to make its support for floating-point or 64-bit value formatting optional so people who don't need that functionality don't pay the cost. It may be difficult to maintain as the function is basically a complex switch statement, which will get ugly with conditional compilation, but that's not a key blocker.

Then that one core formatter can be used by shell_fprintf(), log_printf(), and any application code that happens to need this functionality.

But if a goal is indeed to have only one stream-output-formatter facility in Zephyr, then printk() would also end up using this. Which means the stack usage for core kernel functions that use printk() would depend on application-provided configuration.

Is that not a concern from the kernel perspective?

[andrewboie]

However, it's been made very clear that the owner of lib/os doesn't want it there, nor to use it to make printk() functionally equivalent.

What I don't want is two printf() implementations in our OS library that as a maintainer I have to support. I am not trying to be difficult or give you a hard time, or give the impression this some kind of turf war. I will always consider having two pieces of code doing the fundamentally the same thing to be technical debt, especially if they are in the core OS library. We can do better than this.

I am not opposed to making printk() functionally equivalent, but the naive approach of just defining it in terms of z_prf() will not work because the z_prf() code is far less stack efficient. This is a mechanistic not a policy problem.

Which means the stack usage for core kernel functions that use printk() would depend on application-provided configuration. Is that not a concern from the kernel perspective?

Have you considered that it may be possible to build this in such a way that handling format strings without floating point format codes in them might not need significant extra stack space, even if the code is compiled in? I think you are being overly dismissive of the possibility of making printk() work for these use-cases. The z_prf() code is downright wasteful in what it uses for even simple strings. If we could find a suitable low-footprint replacement for minimal libc I'd delete the whole thing in a heartbeat and get ourselves out of the C library business entirely.

Zephyr applications need to support generating 64-bit integers and floating point in formatted output, certainly from the shell and arguably from logging.

Let's be clear here, we're talking about streamable printf without buffering which is not offered by libc.

All the use-cases in the kernel I see for such streamable printf are for:

1. The debugging shell
2. Logging to text with runtime rendering of the log strings.

We literally don't care if applications need this, z_prf() is a private namespace and is not application facing, and printk() is documented as not being a full printf() implementation. Apps can roll their own or use the C library. This is seriously not an application concern, this is internal kernel business.

z_prf() provides functionality that is crucial to consistent and complete formatting support in the shell, and possibly the logger, and possibly tracing, all of which invoke it under various conditions.

Crucial is a strong word. I think you're overstating the case. If you can afford the overhead of a debug shell, or runtime rendering of text logs, would a line buffer so that you can use libc string rendering functions really kill you? Especially if you're already willing to accept the order-of-magnitude increase in stack space used by z_prf()?

Changes like re-architecting the shell or trying to make vprintk() support both kernel and application needs are not in my wheelhouse and somebody else can pick them up after one of the above choices is taken

Just re-open the original bug and a new assignee will be found if you aren't willing to work on this further.

[pabigot]

I will always consider having two pieces of code doing the fundamentally the same thing to be technical debt, especially if they are in the core OS library. We can do better than this.

Agreed.

If you can afford the overhead of a debug shell, or runtime rendering of text logs, would a line buffer so that you can use libc string rendering functions really kill you?

Perhaps not, but I don't think it's generally possible to determine statically how big a buffer is needed, given that (shell) format operations come from independent modules and produce text that can be short or long depending on arguments. All libc formatting API is all-or-nothing (or "what fits and throw away the rest"). Also it makes application capability dependent on underlying libc selection, which has historically been a problem for output formatting.

We literally don't care if applications need this, z_prf() is a private namespace and is not application facing, and printk() is documented as not being a full printf() implementation.

From a kernel perspective, absolutely. From a Zephyr perspective, it's important to consider the functionality applications need. To that end:

All the use-cases in the kernel I see for such streamable printf are for:

1. The debugging shell
2. Logging to text with runtime rendering of the log strings.

Yes, I think the distinction between what "core kernel" needs and what an application needs is important. So is whether it's important to keep printk()'s resource use independent of configuration options.

• Is all shell use part of core kernel, or is there a specific set of shell modules that are core kernel while others are not? I wouldn't expect the sensor commands to be core kernel...?
• Is subsys/logging core kernel?

Because I'm less concerned about duplication when it's partial functional overlap in functions with clearly-defined different roles and use cases. If:

• z_vprintk is the small tight solution that we have today, with constant resource usage independent of Kconfig and a clear understanding that it doesn't support flowing point, won't handle 64-bit decimal correctly (but will do 64-bit hex), and is to be used for anything that's core kernel;
• other_formatter is a full-featured generator-style formatter derived from existing z_prf(), intended to be compatible with the full range of standard *printf format specifications, and provided by Zephyr as an option for applications that want to make use of its functionality

then is there a conflict? Why not remove z_prf() from core kernel without changing z_vprintk)? Then let applications chose via Kconfig whether they want shell_printf to use z_vprintk() or other_formatter(), which could be moved somewhere other than lib/os to make clear it's not core kernel?

It might even be possible to extend the full capability to subsys/logging with per-call flag that indicates the full formatter should be used for that message, falling back to z_vprintk() if that feature isn't enabled.

[pabigot]

Added dev-review to discuss #29203 (comment) and whether there's consensus that a generic consistent formatting utility has value separate from printk. This is not intended to bypass any maintainer decisions related to what's in core kernel, but if nobody wants this capability the way forward is made more clear.

[pabigot]

Following dev-review 2020-10-22: I'll look into the impact of supporting a single stream-formatting API that can be used by the kernel and applications.