search

teslaworksumn / HeadMaster

DMX parsing and I2C output for the master Animatronic Heads controller
MIT License
2 stars 0 forks source link

I2C Fix #57

Closed CrasherCourse closed 11 years ago

CrasherCourse commented 11 years ago

I believe that these are all the changes we needed from hacky. Please notify me if there is something amiss.

taylortrimble commented 11 years ago

I think we could pick a better name for waitForSPIF. To match coding conventions, how about I2CWaitForTransmission or something? Also, I kind of would like the buffer first.

taylortrimble commented 11 years ago

Also, I don't think we'll ever have a situation where we send a byte without waiting for the transmission to complete. So let's move the waiting after byte transmissions to inside I2CSendByte.

taylortrimble commented 11 years ago

For the rest of style, can we move the waiting function to the bottom since it's not that interesting, forward declare each of the functions after the Code banner, and fix the curly brace on I2CSendByte's implementation?

taylortrimble commented 11 years ago

Functionally: let's convert to using the slave's number as it's address. It will prevent a lot of pointer swapping in the PIC. After we do that, both of the global variables in I2C.h are invalid.

CrasherCourse commented 11 years ago

Actually I named it waitForSPIF because sending the start and stop condition will trigger the interrupt. Meaning the function is used for more than waiting for a transmission. Maybe someone has a better name for this or I just copy and paste the code to replace the function itself.

taylortrimble commented 11 years ago

Technically it's "transmitting" START and STOP conditions as well, so the "wait for transmission" language works all right. Other suggestions welcome too.

Let's forward declare functions at the top of the code block and merge our wait function into sending a byte now (discussed above)

Thanks @CrasherCourse!

CrasherCourse commented 11 years ago

I actually put the waiting function into the header. Instead of declaring it at the top of the code block.

I still have the wait function after the start and stop transmissions since the don't call I2CsendByte.

taylortrimble commented 11 years ago

It all looks good! Our coding conventions say to re-declare all the functions again after the code banner, which is dumb. So you can leave them out and I'll change the coding conventions.

If you tidy up the I2C.h bottom to look more like the Defines section and remove the now-useless Global Variables banner, this should be neat enough to merge.

CrasherCourse commented 11 years ago

Merging soon. Speak now or forever hold your peace.

taylortrimble commented 11 years ago

Okay, I fixed the things I still wanted changed:

PLUS one important change: I2CSendByte and I2CWaitForTransmission aren't really user functions. As such, they aren't part of an API per se and don't belong in the header file— which was the original rationale behind placing the function declarations at the top of the code banner. Does this make sense?

Just remembered that old reasoning now!

CrasherCourse commented 11 years ago

@tylrtrmbl Makes perfect sense.

CrasherCourse commented 11 years ago

But now I got this DETACHED HEAD: branch thing my Git Hub application and I can't go back to the I2Cfix branch.

Also, the curly braces of I2CSend should be aligned. I can't fix it at the moment do to stupid stuff.

Edit: Never mind just fixed that issue.

CrasherCourse commented 11 years ago

Now I can't sync for some reason (behind a commit?)

CrasherCourse commented 11 years ago

@tylrtrmbl you might want to put the finishing touches for me.

My local branch has become buggy and won't sync.

taylortrimble commented 11 years ago

try git pull origin I2Cfix at the command line

CrasherCourse commented 11 years ago

:smile: It worked!

Also, I accidentally learned how to do that smile icon.