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

Fix: s32k3xx: Remove the single assert #1900

Merged
merged 1 commit into from
Aug 18, 2024

Conversation

ALTracer
Copy link
Contributor

@ALTracer ALTracer commented Aug 17, 2024

Detailed description

  • No new features.
  • The existing problem is that the S32K3xx driver, introduced in Add debug and flash support for S32K344 #1652, references assert() which pulls fiprintf() and _vfiprintf_r again, resulting in extra 1.5 KiB of newlib stdio.
  • This PR solves it by replacing the assert with a logged runtime error. IIUC target flash layer should call it with a len=writesize. Depending on review and API guarantees I can drop even that. Update: dropped.

I expect a significant size-diff from CI. This should restore the effect of my previous #1513.
Cannot test on hardware. CC @via.

Your checklist for this pull request

Closing issues

@dragonmux dragonmux added this to the v2.0 release milestone Aug 18, 2024
@dragonmux dragonmux added Enhancement General project improvement Regression Bug caused by a regression labels Aug 18, 2024
@ALTracer ALTracer force-pushed the fix/s32k3xx-no-asserts branch from 9ce0619 to 6a03f81 Compare August 18, 2024 10:17
@ALTracer ALTracer changed the title Fix: s32k3xx: Replace assert with runtime logged error Fix: s32k3xx: Remove the single assert Aug 18, 2024
* Pulling __assert_func() and fiprintf() costs ~1.5 KiB of BMF flash
* Target flash layer is guaranteed to call this with len=writesize anyways
@ALTracer ALTracer force-pushed the fix/s32k3xx-no-asserts branch from 6a03f81 to cc33cb4 Compare August 18, 2024 10:24
Copy link
Member

@dragonmux dragonmux left a comment

Choose a reason for hiding this comment

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

Given the target Flash API layer guarantees this property already - LGTM, merging once the builds complete. Thank you for the contribution!

@dragonmux dragonmux merged commit cc33cb4 into blackmagic-debug:main Aug 18, 2024
26 checks passed
@ALTracer ALTracer deleted the fix/s32k3xx-no-asserts branch August 20, 2024 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement General project improvement Regression Bug caused by a regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants