japaric / stm32f103xx-hal

HAL for the STM32F103xx family of microcontrollers
Apache License 2.0
115 stars 40 forks source link

Fix safe_packed_borrows error #32

Closed lnicola closed 6 years ago

lnicola commented 6 years ago

Fixes #31

japaric commented 6 years ago

Thanks for the PR. In this case I think it would be better just to remove the repr(packed) from the struct. It's not required for this to work; it was just used to work around a bug in rustc that misallocates statically allocated memory (rustc puts variables in .data, instead of in .bss and this wastes flash and makes initialization longer). None of this matter much in any case because the Buffer abstraction is going to be phased out in favor of safe &'static mut references.

Could you update the PR to drop the repr(packed) attribute?

lnicola commented 6 years ago

Done.

Not sure if it matters, but the a stripped release blinky binary was 66 Kb both before and after this change. I'm using openocd to flash it and it reports:

before:
wrote 2048 bytes from file target/thumbv7m-none-eabi/release/examples/blinky in 0.177486s (11.268 KiB/s)

after:
wrote 1024 bytes from file target/thumbv7m-none-eabi/release/examples/blinky in 0.099451s (10.055 KiB/s)

I'm not sure how relevant is this, or why there's such a large discrepancy between the ELF size and what openocd says.

japaric commented 6 years ago

was 66 Kb

Is that the output of ls? That's irrelevant to the actual flash usage. The proper binary size metric to measure is the output of the size command:

$ arm-none-eabi-size target/thumbv7m-none-eabi/release/examples/blinky
   text    data     bss     dec     hex filename
   1024       0       4    1028     404 target/thumbv7m-none-eabi/release/examples/blinky

text is Flash. bss + data is static RAM usage (excludes stack and heap usage).

Thanks for the update!

@homunkulus r+

homunkulus commented 6 years ago

:pushpin: Commit e0294e6 has been approved by japaric

homunkulus commented 6 years ago

:hourglass: Testing commit e0294e60f88fd31dca8acd4e614dce81fd284bbf with merge 8bcb4dd5f0557aa158624d50f8bf1a6e1ef0a40b...

homunkulus commented 6 years ago

:broken_heart: Test failed - status-travis

japaric commented 6 years ago

@homunkulus r+

homunkulus commented 6 years ago

:pushpin: Commit 43de5ed has been approved by japaric

homunkulus commented 6 years ago

:hourglass: Testing commit 43de5ed869220dbca817d7555ea0931e95d270d3 with merge 4338e167fc0233d25acd9c65b525e0f864b0a86f...

homunkulus commented 6 years ago

:sunny: Test successful - status-travis Approved by: japaric Pushing 4338e167fc0233d25acd9c65b525e0f864b0a86f to master...

lnicola commented 6 years ago

The proper binary size metric to measure is the output of the size command

Right, thanks. I had a nagging feeling there was another way to check it 😄 .