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

message consolidation and more use of validators #6409

Merged
merged 3 commits into from
May 20, 2022

Conversation

dhalbert
Copy link
Collaborator

@dhalbert dhalbert commented May 19, 2022

Closes #6392.

To reduce firmware size, this PR uses argument validators where possible and consolidates and simplifies various error messages.

  • I went through all the mp_raise calls, and used validators from argcheck.c to simplify the validation. A few new validators were added, building on keypad: support for vector and matrix key scanning #4891.
  • Added mp_arg_error_invalid(qstr), which raises ValueError with the message "Invalid %q". This is useful many places, where there were previously more specialized but not necessarily more informative messages. Then also added raise_ValueError_invalid_pin() and raise_ValueError_invalid_pins(), which refactor mp_arg_error_invalid(MP_QSTR_pin) and mp_arg_error_invalid(MP_QSTR_pins), both of which are very common (a couple dozen uses each).
  • extmod/vfs_fat.c: refactor multiple identical calls to mp_raise_OSError()
  • ps2io: regularize using clock* instead of clk*, to make argument names and internal names more consistent.
  • Shorten some mp_raise_NotImplementedError()
  • Used %q args to refactor and share some messages.
  • Use m_malloc_fail() instead of more chatty specialized messages which were not necessarily more helpful. Thanks @tannewt for pointing this routine out.
  • PixelBuf.c: refactor some identical error messages to reduce code size.

I did not try to squeeze the last drop out of various _bleio messages, since we are not short on code space in those builds, and the savings would be minimal.

Some initially measured byte savings (current savings are a few bytes more than this due to further changes):

Board Savings in bytes
Trinket M0 316
Metro M0 424
CPX 420
Metro M4 1420
Feather nRF52840 924
Feather RP2040 4524
Feather STM32F405 3948
Metro ESP32-S2 4176
Feather i.MX 1062 5632

I think some of the larger differences here are because some builds are not using LTO.

The number of distinct messages in circuitpython.pot went from 1061 to 989.

Copy link
Member

@gamblor21 gamblor21 left a comment

Choose a reason for hiding this comment

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

Great work! I had one comment where I think an if may still be able to be removed but I may be wrong. I didn't see anywhere else where the values changed.

I wonder if there may be a few new questions where it does not say why a pin is invalid (e.g. not a DAC pin) and now just says invalid. But probably worth the space savings.

ports/mimxrt10xx/common-hal/busio/UART.c Outdated Show resolved Hide resolved
@dhalbert
Copy link
Collaborator Author

I wonder if there may be a few new questions where it does not say why a pin is invalid (e.g. not a DAC pin) and now just says invalid. But probably worth the space savings.

If you look at the backtrace and the line in question, I think it will be clear it's invalid because of the context. There were already cases where the messages just said "Invalid pin" without further explanation; this just takes advantage of regularizing that.

@dhalbert dhalbert requested a review from tannewt May 20, 2022 04:05
jepler
jepler previously approved these changes May 20, 2022
Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

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

Thanks! I think it could go in just like this, but there are a few items I spotted along the way.

ports/atmel-samd/common-hal/alarm/pin/PinAlarm.c Outdated Show resolved Hide resolved
ports/atmel-samd/common-hal/busio/I2C.c Outdated Show resolved Hide resolved
ports/atmel-samd/common-hal/pulseio/PulseIn.c Show resolved Hide resolved
ports/raspberrypi/common-hal/pulseio/PulseIn.c Outdated Show resolved Hide resolved
ports/stm/common-hal/busio/SPI.c Show resolved Hide resolved
ports/stm/common-hal/busio/UART.c Outdated Show resolved Hide resolved
ports/stm/common-hal/canio/CAN.c Outdated Show resolved Hide resolved
shared-bindings/ps2io/Ps2.c Show resolved Hide resolved
shared-bindings/sdioio/SDCard.c Outdated Show resolved Hide resolved
shared-module/bitbangio/SPI.c Outdated Show resolved Hide resolved
@dhalbert
Copy link
Collaborator Author

With the latest round of changes, Trinket M0 is now up to 544 bytes saved.

@dhalbert dhalbert requested a review from jepler May 20, 2022 14:12
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Awesome work! Let's merge this and tag 8.0.0-alpha.0 since it changes some exception types.

@dhalbert
Copy link
Collaborator Author

dhalbert commented May 20, 2022

Let's merge this and tag 8.0.0-alpha.0 since it changes some exception types.

I agree. Then we can merge the postponed 8.0.0 PR's, and implement the hanging 8.0.0 issues.

@dhalbert dhalbert merged commit 491374c into adafruit:main May 20, 2022
@dhalbert dhalbert deleted the messages-refactor branch May 20, 2022 19:25
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.

Consolidate error messages and use common validation routines
4 participants