Optimized G3Firmware - Githubissues

craiglink commented 12 years ago

These are some changes that optimize the AVR pin setting down to single instruction calls for Ports A-G. It also fixes an interrupt pin setting bug that existed - http://code.google.com/p/arduino/issues/detail?id=146 . Most of this work comes from stuff I did earlier in the year on the Arduino with the digitialWriteFast macros

The core of this checkin use the use of a template class, PinTmplt, to replace the previously existing AvrPort and Pin files. The use of the template allows for the port / pin number of be determined at compile time so that a single sbi or cbi instruction can be used to set or clear the bit. The existing code used a dozen or more instructions to accomplish the same thing. It results in a 20% savings CPU in each main loop iteration while moving a stepper motor.

Additionally I abstracted the display portion of the InterfaceBoard code into an abstract Display class so that other displays could easily be added to the firmware. Included is a Modtronix interface using i2c. I made a shield that remapped the Arduino's i2c pins so that I could use them asynchronously.

Have a look and let me know if you have any questions.

FarMcKon commented 12 years ago

Looking at this now. Great set of patches, it might take a bit to dig through.

FarMcKon commented 12 years ago

Hey Craiglink, I did a quick run through. It looks like there is some nifty stuff in that pull request, some things I don't totally follow, and some customization for your own bot in there.

Thanks for the pull request, but the way this is bundled it'll be a bit of work to tease it apart. I see 3 categories of changes 1) Abstraction simplification: Looks nifty, but I'd like to know the real time/speed savings before I grab it and go 2) some refactoring and renaming: happens as part of hacking, but probably we don't need that renaming. 3) some custom to Craigbot stuff. We don't really want much of that as awesome as your custom mods are :)

Thanks for the pull request. if you want to send a more granular pull for some key stuff, we could get to that sooner. Otherwise I'll throw splitting this up and grabbing the parts we want into our (somewhat long) backlog.

craiglink commented 12 years ago

I actually don't think there was any refactoring or renaming in the code. I did introduce a couple of new classes ( actually templates ). PinTmplt, StepperTmplt, a few derived templates ( whether there are endstops or not ) and Display. Though I have another branch for my personal tweaks ( CraigBot ), I don't believe any of these are actually in the checkin. I added the Display base class as a way to abstract out the actual Display from the LiquidCrystal display class. I did add support for the Modtronix LCD2S display. Feel free to remove it and the associated twi.* files for communicating with it. I'd love if you keep the Display class though.

The StepperTmplt templates where named that because they are template classes and not just the previous StepperInterface class. Specifically these classes use PinTmplt classes to define the pins that they operate on at compile time and NOT at run time. This allows the compiler to significantly reduce the amount of code requires to interface with the stepper motors. By deriving 2 classes ( one with endstops and one without ) from this template, the compile time code can be further optimized based on the fact that we know that steppers A and B do not have endstops ever.

The PinTmplt class expands on what ones once AvrPort.hh and has been broken into separate files AvrPort.* and Pin.*. This template allows for the compiler to determine at COMPILE time the actual AVR pins being used and allows the compiler to optimize the pin setting to single instruction calls ( sbi, cbi ). The previous AvrPort code compiled to something like this

000056c0 <_ZN3Pin8setValueEb>: 56c0: fc 01 movw r30, r24 56c2: a0 81 ld r26, Z 56c4: b1 81 ldd r27, Z+1 ; 0x01 56c6: 12 96 adiw r26, 0x02 ; 2 56c8: 4c 91 ld r20, X 56ca: 12 97 sbiw r26, 0x02 ; 2 56cc: 82 81 ldd r24, Z+2 ; 0x02 56ce: 8f 70 andi r24, 0x0F ; 15 56d0: 21 e0 ldi r18, 0x01 ; 1 56d2: 30 e0 ldi r19, 0x00 ; 0 56d4: 02 c0 rjmp .+4 ; 0x56da <_ZN3Pin8setValueEb+0x1a> 56d6: 22 0f add r18, r18 56d8: 33 1f adc r19, r19 56da: 8a 95 dec r24 56dc: e2 f7 brpl .-8 ; 0x56d6 <_ZN3Pin8setValueEb+0x16> 56de: 92 2f mov r25, r18 56e0: 66 23 and r22, r22 56e2: 11 f0 breq .+4 ; 0x56e8 <_ZN3Pin8setValueEb+0x28> 56e4: 82 2f mov r24, r18 56e6: 01 c0 rjmp .+2 ; 0x56ea <_ZN3Pin8setValueEb+0x2a> 56e8: 80 e0 ldi r24, 0x00 ; 0 56ea: 90 95 com r25 56ec: 94 23 and r25, r20 56ee: 89 2b or r24, r25 56f0: 12 96 adiw r26, 0x02 ; 2 56f2: 8c 93 st X, r24 56f4: 08 95 ret

The PinTmplt code compiles to this.

00004e6a <_ZNK12StepperTmpltILh1E8PinTmpltILj32ELh2EES0_ILj32ELh3EES0_ILj32ELh1EEE4stepEb>: 4e6a: 66 23 and r22, r22 4e6c: 11 f0 breq .+4 ; 0x4e72 <_ZNK12StepperTmpltILh1E8PinTmpltILj32ELh2EES0_ILj32ELh3EES0_ILj32ELh1EEE4stepEb+0x8> 4e6e: 13 9a sbi 0x02, 3 ; 2 4e70: 08 95 ret 4e72: 13 98 cbi 0x02, 3 ; 2 4e74: 08 95 ret

Additionally it fixes a bug that could occur in the AvrPort code if the bit was being set on Ports H and up and an interrupt occurred.

Feel free to ask some more questions if you have some.
-Craig

P.S. I am occasionally encounter some odd tabbing in some of the lines of the files. It seems like someone is editing the files with tabstops set to 8 characters which is definitely not an industry standard.

-----Original Message----- From: Far McKon [mailto:reply@reply.github.com] Sent: Wednesday, November 30, 2011 3:35 PM To: Craig Link Subject: Re: [G3Firmware] Optimized G3Firmware (#83)

Hey Craiglink, I did a quick run through. It looks like there is some nifty stuff in that pull request, some things I don't totally follow, and some customization for your own bot in there.

Thanks for the pull request, but the way this is bundled it'll be a bit of work to tease it apart. I see 3 categories of changes 1) Abstraction simplification: Looks nifty, but I'd like to know the real time/speed savings before I grab it and go 2) some refactoring and renaming: happens as part of hacking, but probably we don't need that renaming. 3) some custom to Craigbot stuff. We don't really want much of that as awesome as your custom mods are :)

Thanks for the pull request. if you want to send a more granular pull for some key stuff, we could get to that sooner. Otherwise I'll throw splitting this up and grabbing the parts we want into our (somewhat long) backlog.

Reply to this email directly or view it on GitHub: https://github.com/makerbot/G3Firmware/pull/83#issuecomment-2968523

craiglink commented 12 years ago

Also just to clarify the example below more, the PinTmplt code will actually compile down to a single instruction if the value is known at compile time - so must of the setValue() or setDirection() calls compile to a single instruction:

4e6e:   13 9a           sbi 0x02, 3 ; 2

or 4e72: 13 98 cbi 0x02, 3 ; 2

FYI. I also have a number of other fixes and perf improvements coming your way that I need to finish documenting. Let me know if it's better for me to submit smaller pull request instead a large batch.

-----Original Message----- From: Craig Link Sent: Wednesday, November 30, 2011 8:34 PM To: Far McKon Subject: RE: [G3Firmware] Optimized G3Firmware (#83)