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

crc32: Buffer up log output to avoid prefix-chunking, fix func #1998

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ALTracer
Copy link
Contributor

Detailed description

  • This is a fix to one of my edited/contributed features regarding compare-sections via qCRC.
  • There are three existing problems, which are not critical to upstream BMD here.
  • This PR updates crc32 dispatch code to deal with them.
  1. Because of Fix: Replace attribute alias in CRC32 #1713 and requirements to support Windows and MacOS (cross-platform portable software), the __func__ portion of log became identical as collateral damage. This change restores the behaviour intended in Fix: qCRC performance uplift #1708, so that we can tell apart speed reports from BMF+ENABLE_DEBUG and BMDA.
  2. On out-of-tree platforms like Farpatch which use BMD as a git submodule directly, custom logger implementations format every DEBUG_INFO() call etc. by prepending loglevel and timestamp, so the 2-3 piece logmessage gets broken and mixed. This change buffers the optional output pieces into a zeroed stack buffer via snprintf() to avoid that.
  3. Xtensa Farpatch crashes on every divide-by-zero, which can happen here when <1 milliseconds are reported elapsed by corresponding platform timing code. BMF on Cortex-M3 does not trap integer divide-by-zero into UsageFault, so it was never affected. BMDA usually gets more than 1ms due to USB FS delays and all the layer overheads. This change skips the speed report by checking for both length and time to be big enough.

Your checklist for this pull request

Closing issues

Fixes some issues in Farpatch.

src/crc32.c Outdated
}
DEBUG_INFO("\n");
loglen += snprintf(&logbuf[loglen], logcap - loglen, "\n");
DEBUG_INFO("%s", logbuf);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you roll the "\n" into the DEBUG_INFO() here? I could see there being a case where we replace all instances of DEBUG_x("...\n" with something else, assuming BMP ever moves to a logging infrastructure that adds newlines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, applied. So log entries would stored in some circular dmesg buffer, separated with \0 not \n, and the consuming function would format numeric millisecond timestamps and potentially multiline messages with e.g. \r\n if needed? I don't remember how NuttX ramlog driver works, but the formatting is up to configurable syslog(), not callsites; and in BMF we didn't have that.

@xobs
Copy link
Contributor

xobs commented Nov 11, 2024

I like this change! It improves readability when doing checksum benchmarking.

* Format the optional output pieces into a stack buffer, then do a single DEBUG_INFO() call
* Restore the name of compiled function implementation in the log output
* Avoid dividing by 0 milliseconds elapsed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants