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

Optimise BasicMode::clear RAM usage and optimize size of Ssd1306::char_to_bitmap #195

Merged
merged 12 commits into from
Oct 9, 2023
Merged

Optimise BasicMode::clear RAM usage and optimize size of Ssd1306::char_to_bitmap #195

merged 12 commits into from
Oct 9, 2023

Conversation

alexey-medvedchikov
Copy link
Contributor

@alexey-medvedchikov alexey-medvedchikov commented Sep 20, 2023

Hi! Thank you for helping out with SSD1306 development! Please:

  • Check that you've added documentation to any new methods
  • Rebase from master if you're not already up to date
  • Add or modify an example if there are changes to the public API
  • Add a CHANGELOG.md entry in the Unreleased section under the appropriate heading (Added, Fixed, Changed, etc)
  • Run rustfmt on the project with cargo fmt --all - CI will not pass without this step
  • Check that your branch is up to date with master and that CI is passing once the PR is opened

PR description

This PR fixes issues with small AVRs:

  • BasicMode doesn't work on MCUs with less than ~1.5Kb of RAM because there's a stack array of 1024 bytes allocated to zero-out pages. Instead, we can zero-out in small pieces.
  • Refactor glyph search to use lookup table instead of one big match. Saves around 3.5Kb of compiled code on AVR ATMega-s. Didn't test on other architectures, but pretty sure it will save some size too.

AVR size difference

Before:

text    data     bss     dec     hex filename
   0    8446       0    8446    20fe object.hex

After:

text    data     bss     dec     hex filename
   0    4988       0    4988    137c object.hex

STM32 (thumbv7em) size difference

Before:

verified 7212 bytes

After:

verified 6392 bytes

Copy link
Collaborator

@jamwaffles jamwaffles left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the improvements! The memory size reductions are impressive. I left a few questions on your clear() changes.

I also want to test this PR on some hardware before merging but I'm travelling for this week. I hope you don't mind the delay.

src/mode/mod.rs Outdated
Comment on lines 37 to 39
for _ in 0..dim.0 * dim.1 {
self.draw(&[0; 8])?;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've got a couple of questions here. I hope you don't mind a short list:

  1. Because you're sending 8 bytes in a row, shouldn't this be 0..(dim.0 * dim.1) / 8?
  2. Could we increase the slice size to 64 or 128 bytes? This will still save a decent amount of memory but will reduce the overhead of doing the CS pin setup, etc.
  3. To add to (1.), I think you might need a further / 8 because the display's dimensions are in pixels, but a page is 8 bytes tall.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the quick reply! Yes, it all does make sense, I'll address comments later today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed the comments you provided. Also, I realised that the addressing mode must be set and restored, so did that too.

@alexey-medvedchikov
Copy link
Contributor Author

alexey-medvedchikov commented Sep 21, 2023

@jamwaffles No problemo!

Although I saved some space, I still can't squeeze my program into atmega168 - bme680 driver is a resource hog (making a small weather station with a screen). So I'll try to switch to a STM32 MCU, I can test this change there too later this week. I also have ESP8266 at hand, but I've never used it with Rust, so not sure if I can quickly check it.

@alexey-medvedchikov
Copy link
Contributor Author

alexey-medvedchikov commented Sep 21, 2023

LOL, I've just noticed that all the first and last bytes of each glyph in the font are zeros, so I just made the data array smaller by another 500 bytes. It is a bit clunky, but the font is hardcoded (and quite nice), so I don't think it is a problem. -3.5Kb total on atmega.

@alexey-medvedchikov
Copy link
Contributor Author

Tested on NUCLEO-F446ZE and ATMega168 with 128x64 and 128x32, all work fine.

@alexey-medvedchikov
Copy link
Contributor Author

Got rid of extra-sized arrays in Command::send, cuts 80 bytes on Cortex-M4 and 260 bytes on ATMega168.

Copy link
Collaborator

@jamwaffles jamwaffles left a comment

Choose a reason for hiding this comment

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

Thanks very much for testing the different display sizes - that's saved me some time :). I've left a couple of inline suggestions which make the code a bit cleaner. Let me know what you think.

src/mode/mod.rs Outdated Show resolved Hide resolved
src/mode/terminal.rs Outdated Show resolved Hide resolved
'~' => [0x00, 0x02, 0x01, 0x02, 0x01, 0x00, 0x00, 0x00],
_ => [0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00],
}
const CHARS: [[u8; 6]; 95] = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice optimisation! In hindsight it was silly to store a pile of extra zeros but thanks for the fix!

CHANGELOG.md Outdated Show resolved Hide resolved
@alexey-medvedchikov
Copy link
Contributor Author

Thanks for the review. I accepted all your suggestions, a newbie in Rust, so make simple mistakes. Also, the merge conflict should be resolved now.

Copy link
Collaborator

@jamwaffles jamwaffles left a comment

Choose a reason for hiding this comment

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

Hey, sorry for the review delay. I've got one more nit that hopefully doesn't fail the tests any more, then this is good for merge. Thanks for your patience!

src/mode/terminal.rs Outdated Show resolved Hide resolved
Co-authored-by: James Waples <[email protected]>
@alexey-medvedchikov
Copy link
Contributor Author

@jamwaffles checked and committed your changes. Thanks for spending so much time on this one!

Copy link
Collaborator

@jamwaffles jamwaffles left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks again for your patience :)

@jamwaffles jamwaffles merged commit fddf721 into rust-embedded-community:master Oct 9, 2023
@jamwaffles
Copy link
Collaborator

This is released in v0.8.3. cargo update should pull in the changes.

@alexey-medvedchikov alexey-medvedchikov deleted the size-optimization branch October 10, 2023 12:51
@bugadani
Copy link
Contributor

FYI I'm firmly opposed to e309b41 as it means I'll have to duplicate the whole command table for the async interface.

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.

3 participants