-
Notifications
You must be signed in to change notification settings - Fork 4
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
should we talk about reserving levels? #14
Comments
I can imagine that this feature could be useful in some cases, but I do not recommend to use Can we use log level
and the output could be:
Of course it will look nice on terminals (file viewers) which have always the same character width (so the letter 'w' has the same width as the letter 'i'). |
that's not what I meant, but close. I meant the
Actually I was thinking of dedicating a whole octal level to special punctuation where at least 4 levels are devoted to different quantity indentations RF24LOGGER_info("Weather", "The parameters are below:");
RF24LOGGER_log(1, "Weather", "temperature %f", 23.5);
RF24LOGGER_log(2, "Weather", "wind speed %f", 10);
RF24LOGGER_log(3, "Weather", "wind direction %s", "SE"); would output like:
above demo is using 8-space tab characters. |
Ok, I see. Good point. That may happen. On multithread operating system multi-line log messages may be a pain because they may mix together and appear in an "undefined" order. They should be logged under the same vendor id - in my opinion. How do you distinguish which multiline message belong to which master message? With this punctuation? Then every single thread should use a different punctuation (log level). That may be hard to manage, because some piece of code may be called from different threads. Keeping log messages small to one line eliminates those problems. You could also try to construct log messages with
should something like:
From the logging perspective this is just a "one-line" log message and will be displayed in one call. But if you want to display 200 elements in a loop then we have a nut to crack. Normally in Java I would use:
|
Since we're already parsing through the format c-str one char at a time, we could also replace any I also have a |
I just refactored the printf parsing a bit more, so now all platforms enjoy the same specifier support... I was able to combine from the modified gettingStarted.ino RF24Log_log(077, vendorID, "This\n\tis a multiline\n\t\tmessage that\n\tends with a\nblank line\n\n");
RF24Log_log(75, vendorID, "%%%%This is level 0x%02x (0b%08b or%4lu)%3c", 75, 75, 75, '!'); Output from above code
This tactic also extends the macro |
Maybe the level 0 should be reserved as the "broadcasting" channel in which the header's log level description is skipped. With the ability to disable timestamps and the user can pass a zero-length string as the vendorID, its the closest thing we can offer to a truly blank line in the logs. I could also be able to #ifdef ARDUINO
Serial.println("");
#else
std::cout << std::endl;
#endif |
@2bndy5 I see that you did quite a good job. It fixes some issues and introduces new features. Could you make a PR so we can make a review? The idea with To print blank lines or disable timestamps - can we use some kind of new format - instead of reusing log levels for this:
With this idea the log level will be preserved and some developer may implement a very custom log handler where multiline feature will be just ignored and printed as a standard log message. |
Instead of having the parser look for a The only thing we would need to turn off is the log level and timestamp. The const char PROGMEM DisableVendor[] = ""; // I'm currently detecting empty strings and skipping the delimiter
RF24Log_log(0, DisableVendor, ""); // this is now a do while loop, so empty messages still get a header The only other way to disable a timestamp is through a macro called The only use case I can think of needing the entire textual real-estate of a log line is for dumping a formatted file (like JSON for example). This is why its easier to just add an extra |
@wmarkow you can comment on commits, open a PR to the refactor branch to add your changes... I originally set out to get this building on PC and PicoSDK (using CMake). So I'm not yet ready to submit a PR for review (sorry).
This is also disabled when the macro |
I think providing (for any purpose - including disabling vendor id) an empty string as vendor id should be not allowed. Vendor id should be always provided as not empty so the log handler may use it to provide more sophisticated logic related to handling log messages. Disabling of vendor id must be done in a different way. @2bndy5 , what is the status of your #18 pull request against this issue? I meant, does this PR reserve some log levels for special purposes or you did everything configurable by some define switches? |
please checkout the examples. I am using the quoted snippet to prompt the user for input about filtering verbosity. AllLogLevels.ino would be a good example to run.
There are a few new macros but nothing that conditionally reserves a logging level for a certain purpose. Level 0 is hard-configured to skip the timestamp and level description though. |
How would you go about preventing an empty vendorId from being accepted? I've been criticized about documenting something that should be unsupported but not restricting unsupported behavior (mostly concerning RF24 clones/counterfeits). |
Just to clarify: by "disabling" I mean the way that vendor id is not written to the destination stream, however a correct value of vendor should be passed in the API macros. @2bndy5, I have nothing against disabling vendor id, timestamp or other parts of the log message, but it should be done directly in a correct implementation of Preventing empty vendorId may be not easy to do. Normally I would check the argument and throw exception but on Arduino (AVR) we do not have such u thing. Maybe it should be written somewhere in the docs that a good practice is to provide a valid non empty vendorId. The last way would be to modify |
I would like to avoid relying on this. It has become painfully obvious to me that people just don't RTFM (or they don't care to read anything because it isn't a 10 minute youtube video). There's also the language barrier for those that can't fluently read English writing...
This can't be done on 1 line of code because the vendorId must be a variable stored in flash (according to current implementation). I have started an entry point to adding custom logic about parsing/filtering the vendorId. This is an excerpt from AVR's PGM_P id = reinterpret_cast<PGM_P>(vendorId);
char v = pgm_read_byte(id++);
if (v) {
appendStr(vendorId);
appendChar(RF24LOG_DELIMITER);
}
This would seem counter-productive to those that aren't aware of our lib's best practices. I can already foresee the issues complaining about this tactic. I really don't understand the problem with using an empty c-string as a way to disable vendorId (on a per-message basis). I'm only using the null terminating char as a sentinel if there is no char data preceding it. In that sense, it is actually like reserving a special phrase (and position) in the vendorId to trigger a certain behavior (like skipping the vendorId output for that message only). And, this accounts for your undesired behavior to accept/output empty vendorId values. We definitely cannot just use a conditionally defined macro to disable the vendorId because it isn't dynamic enough for runtime.
Technically, an empty c-string is a correct value because it isn't a |
@2bndy5 thank you for your detailed comment.
You have absolute right here. My intention with vendorId is to offer the possibility to catch log messages coming from a specific library and process them in some special way. Example: my project uses two libraries RF24 and RF24Network. I want to pass all DEBUG messages from RF24Network to Serial but I'm not interested in all DEBUG from RF24. With the correct (something significant and not empty, like "RF24" for RF24 lib and "RF24Network" for RF24Network library) vendorId in both libraries I can implement the desired logic. If you put an empty vendorId in some places inside of RF24 library then I will not be able to find out where the specific message comes from (in sucha case I would probably redirect it to Serial). @2bndy5, give me some time - I just want to read the whole issue again to refresh my memory about your scenario. It was about multiline messages, right? |
😆 It was supposed to be a general discussion about reserving log levels for different purposes. I used multi-line messages as an example, but that led to manipulating the header info when a TBH, the only use case that disabling the vendorId should be used is when the user wants to output formatted data to a file (like JSON) or a general user prompt on the Serial terminal. EDIT: this should be
YES! This is a very common application for python's logging lib. I have this kind of implemented with the DualStreams example on the refactor branch (please please please checkout those changes). However its not geared towards multiple libraries... hmm. It sounds like manipulating 2 internally configured log levels for the same output stream would be easier than customizing the vendorId parsing. I think #19 would be a step in that direction. |
I'm assuming that this is the last goal that needs resolution for this lib. I'll be exploring some options (locally), but I don't like the idea of comparing vendorIDs. I think the MSBit in a handler's stored |
The thought occurred to me that we could skip the timestamp if a negative version of the log level were passed to
log()
. What if the user just wants an indented line (withvendorID
) for multi-line messages?We could of course not reserve certain levels and just patch in flags to control the header (kinda like a
printf()
specifier string).The text was updated successfully, but these errors were encountered: