Marlin

Marlin copied to clipboard

Description

• remove template + status_printf_P => status_printf() - marlinui.h
• revert code to 2.1.1.1
• change fmt to pfmt as it is stated
• I want to point out that this line in previous Marlin ver. had va_start(args, FTOP(pfmt));
• • but va_start(args, pfmt); works/compiles normally.
    so Question: was having FTOP there meant for a reason? because it seems to be fine without it.

I was able to make it show, short clip of what is happening for context:

short clip of what is happening for context

---

At some random times I've noticed this strange phenomena that would happen. The status display would show different things, and back tracing to what it could be I narrowed it down to lcd/marlinui.h

  template<typename... Args>
  static void status_printf(int8_t level, FSTR_P const ffmt, Args... more) { status_printf_P(level, FTOP(ffmt), more...); }

when hovering over this function I get this:

and so I think I solved the issue by simply removing the template associated with it. and just simplify the function to one, like it was in v.2.1.1.1 or earlier

Requirements

Noticed with ProUI and JyersUI, not sure about any other.

Benefits

hopefully stops random status messages from appearing.

Configurations

Related Issues

could be an underlying issue that stays hidden until random events.

I have not reviewed this because I honestly do not know when each of these is appropriate: change fmt to pfmt as it is stated

Feel free to educate me and convince me that what you have done is correct.

It's been a longtime convention for classes to have a method that takes a straight PSTR with a "_P" suffix in the method name, and then to have the versions taking other types (such as F-strings) to be overloaded and call back to the P-string version. In the long run it would be cleaner to remove as much of the vestiges of special handling for strings in PROGMEM as we can, and in the most ideal world all code pertaining to strings should use the flexible MString class, but there are good arguments for keeping some of this lower level string stuff around for utility.

Anyway, I believe we should retain the "_P" version of the function in case some user or developer wants to use it to display a PSTR, and we should retain the FSTR version as an "alias" for this function if it doesn't greatly impact the build size (i.e., for smaller boards).

A quick build test shows that the build size with MarlinUI on AVR isn't significantly impacted by the use of the inline passthrough for FSTR => PSTR. Specifically, removing it only reduces the build size by 6 bytes.

Following this PR's suggestion that we need to improve clarity, I've kept the parameter renaming and also applied some more of that to the string classes.

We have to keep using PROGMEM strings for the benefit of AVR, so hopefully we can continue to improve the clarity of our usage conventions, and, where we can, we will continue to transition to MString and SString, since these types have built-in polymorphism that commutes to functions that use them as parameters.

To clarify, we use FTOP wherever there is an F-string (a PROGMEM string wrapped in __FlashStringHelper) that needs to be passed to a function taking a PROGMEM string pointer (PGM_P), or which otherwise needs to be converted to a PROGMEM string pointer.

In most places we've made sure to distinguish functions that expect PROGMEM strings (which differ on AVR) from functions that expect C-strings (which can only be SRAM strings on AVR) and we've transitioned code that took P-strings into code taking F-strings, retaining the P-string versions for utility. And I started the process of moving to MString and SString with a large initial refactor, but these types are not always as efficient or small as plain string functions, so there is some trade-off.

String Stuff

• Functions taking char *, FSTR_P, MString, or SString are polymorphic and need no special naming.
• Functions specifically taking PROGMEM strings should have a name ending in _P.
• Aim to refactor PSTR() to F() throughout the codebase and replace direct calls to myfunc_P.
• Keep an eye out for errors where a char * function parameter should be changed to PGM_P.
• Keep an eye out for errors where a char * SRAM string is being passed to a function expecting a PGM_P (and vice-versa).
• Keep an eye out for printf format strings using "%s" that should really be using S_FMT.

In some instances we've ignored code that neglects the existence of PROGMEM strings, since the affected code was only designed to run on ARM. However, there is no reason to limit code to ARM (especially LCD code) if AVR boards could otherwise run it. So it's a good idea to convert code to use F-strings wherever we possibly can. This will also help to unify the language translations across all displays.

@thinkyhead I notice something which is odd, sometimes using TS (TString) instead of MString does result in less memory usage. and also, using char instead uses less. but sometimes using .set() or .setf() for an MString will use less for setting an LCD status, but when using for something like queue.inject() it uses more.

basically I'm not quite sure the approach to take. stick with char and sprintf_P, or make it quick and easy just use something like ui.set_status(TS(F("text"))