Skip to content

[2.0.x] AT90USB1286 PIO cleanup and optimization #11230

Merged
thinkyhead merged 1 commit intoMarlinFirmware:bugfix-2.0.xfrom
fiveangle:bf2-at90usb_updates
Jul 26, 2018
Merged

[2.0.x] AT90USB1286 PIO cleanup and optimization #11230
thinkyhead merged 1 commit intoMarlinFirmware:bugfix-2.0.xfrom
fiveangle:bf2-at90usb_updates

Conversation

@fiveangle
Copy link

@fiveangle fiveangle commented Jul 9, 2018

-normalize env and board to lowercase naming convention
-make board name follow descriptive convention
-implement -fsingle-precision-constant compile optimization per #11178 (comment)
-fix remnant typo in 5DPRINT entry

@fiveangle
Copy link
Author

Regarding optimization, there's also a bit of storage savings:

Before:

DATA:    [=====     ]  48.6% (used 3979 bytes from 8192 bytes)
PROGRAM: [==========]  97.3% (used 119512 bytes from 122880 bytes)

After:

DATA:    [=====     ]  48.6% (used 3979 bytes from 8192 bytes)
PROGRAM: [==========]  97.2% (used 119460 bytes from 122880 bytes)

@fiveangle fiveangle force-pushed the bf2-at90usb_updates branch 2 times, most recently from 70a7550 to 5738477 Compare July 9, 2018 05:13
@fiveangle
Copy link
Author

@thinkyhead - I haven't noticed any negative fallout with the lower FP precision passing several prints through (stock gcode jobs I have sitting on Octoprint) and they appear to print identically but I only have a cartesian bot to test on. Would we anticipate any appreciable negative fallout with this on other motion systems like delta ? Or is this just the lazy man's way of not scrubbing all literals to add the "t" single suffix and ensuring all double-precision required operations are done with the necessary double-precision calls (e.g. sinf(), etc) ?

I'm not familiar enough with the precision required within Marlin to adequately judge.

@Bob-the-Kuhn
Copy link
Contributor

You'll also need to change the environment name in pins.h

@fiveangle
Copy link
Author

fiveangle commented Jul 9, 2018

Good point Bob ! Reviewing the other env names, perhaps we should go the other way, break from the PIO standard of citing all lower-case and just convert to the chip name "AT90USB1286" ? It just seemed weird when I searched for "at90usb1286" and found "at90USB1286" instead, but reviewing "LPC1768" and "ST32F4" it seems we should either leave them or also switch to "lpc1768" and "stm32f4" ? The pio env:NAME docs claim "a-z", "0-9", and "_" are valid in environment names and everyone else seems to take it to heart, but obviously uppercase in env names work fine.

Perhaps my OCD is getting the better of me, but no matter how you slice it "at90USB1286" just doesn't seem to fit either way :)

@p3p
Copy link
Member

p3p commented Jul 9, 2018

@fiveangle On AVR double is just an alias for float, both 32bit, which is why we have the issue that double literals are used everywhere.. it just didn't matter if you left off the 'f'. Using -fsingle-precision-constant is just to avoid the need to go through and fix them all on ARM based board with actual double support.

Saying that I don't know why you got the binary size difference ;)

@fiveangle
Copy link
Author

@Bob-the-Kuhn - I noticed 2 things: you have typo on the 5DPRINT entry and still have the Teensylu2 on env:teensy20:

  252  #elif MB(TEENSYLU)
  253:   #include "pins_TEENSYLU.h"          // AT90USB1286, AT90USB1286P                  env:at90usb1286_cdc
  254  #elif MB(PRINTRBOARD)
  255:   #include "pins_PRINTRBOARD.h"       // AT90USB1286                                env:at90usb1286_dfu
  256  #elif MB(PRINTRBOARD_REVF)
  257:   #include "pins_PRINTRBOARD_REVF.h"  // AT90USB1286                                env:at90usb1286_dfu
  258  #elif MB(BRAINWAVE)
  259:   #include "pins_BRAINWAVE.h"         // AT90USB646                                 env:at90usb1286_cdc
  260  #elif MB(BRAINWAVE_PRO)
  261:   #include "pins_BRAINWAVE_PRO.h"     // AT90USB1286                                env:at90usb1286_cdc
  262  #elif MB(SAV_MKI)
  263:   #include "pins_SAV_MKI.h"           // AT90USB1286                                env:at90usb1286_cdc
  264  #elif MB(TEENSY2)
  265    #include "pins_TEENSY2.h"           // AT90USB1286                                env:teensy20
  266  #elif MB(5DPRINT)
  267:   #include "pins_5DPRINT.h"           // AT90USB1286                                ?env:at90usb1286_dfu

I think you were struggling to figure out which boot loader it used, but are we okay with removing the "?" and going with DFU ?

On the Teensylu2, according to https://reprap.org/wiki/Teensylu it did not use actual Teensy and so it relies on a user-provided bootloader (which by default from Atmel is DFU and can then never be Paul's proprietary Halfkay). I know it isn't a huge deal since the 9 people running these boards probably haven't changed them in years and the rest of have them sitting in a drawer somewhere collecting dust, but are we okay with switching Teensy2 to at90usb1286_cdc or at90usb1286_dfu just to be on the safe side (of the bootloader) ? :)

[I'll refrain from the sidebar of how confusing it is that this board was named "TEENSY2" (technically a generic MCU) in Marlin instead of "TEENSYLU2"]

@fiveangle
Copy link
Author

Saying that I don't know why you got the binary size difference ;)

Then I say we go with it: 52 bytes is 52 bytes ! 😋

@fiveangle fiveangle force-pushed the bf2-at90usb_updates branch from 5738477 to 5e8e428 Compare July 9, 2018 12:40
@Bob-the-Kuhn
Copy link
Contributor

Consistency in Marlin? I'm against that for historical reasons. 😉

I don't remember where I got the mixed case name for the 1286. It really doesn't matter.

I didn't know there was a "TEENSYLU2". All I could find on it was a github page with the TEENSYLU2 prototype design.

I'm pretty sure TEENSY2 is the Teensy++ 2.0. Way back in 1.0.2 it refers to "Teensy++ 2.0 Breadboard pin assignments" and shows an outline of the Teensy++ 2.0.

5DPRINT - yes, I did not know for sure what bootloader was used on it. As long as pins.h & the comments in platformio.ini are consistent then I'm happy.

-normalize `env` and `board` to lowercase naming convention
-make board `name` follow descriptive convention
-implement `-fsingle-precision-constant` compile optimization per MarlinFirmware#11178 (comment)
-fix typo in 5DPRINT entry
@fiveangle fiveangle force-pushed the bf2-at90usb_updates branch from 5e8e428 to 3d873c1 Compare July 9, 2018 14:17
@fiveangle
Copy link
Author

Consistency in Marlin? I'm against that for historical reasons. 😉

Fair point ! 😛

Okay, popped TEENSY2 back to just a vanilla Teensy++ 2.0 board and I think this is good to go !

@thinkyhead thinkyhead merged commit 8a24ff9 into MarlinFirmware:bugfix-2.0.x Jul 26, 2018
@fiveangle fiveangle deleted the bf2-at90usb_updates branch July 27, 2018 00:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants