teslaworksumn / HeadMaster

DMX parsing and I2C output for the master Animatronic Heads controller

MIT License

2 stars 0 forks source link

DMXReceive now uses special struct #38

Closed taylortrimble closed 11 years ago

taylortrimble commented 11 years ago

This is a complicated initial diff, so play close attention to what is going on!

You'll notice that some stuff (like waiting for a start address) are not implemented because that's going on in other branches. We won't duplicate the efforts here, but we'll wait for those to materialize.

We'll keep working on it all in parallel! Everyone let me know your thoughts! This is a big change.

taylortrimble commented 11 years ago

Okay, I think this should do the following:

Accept partial DMX frames as long as we get the data we need
Utilize a smaller buffer in memory and discard DMX channels we don't need.
Remove use of global variables for DMX code

But, this is likely to be riddled with little bugs I made. So, @aterlumen and @CrasherCourse especially, scrounge it over and let me know what you think.

taylortrimble commented 11 years ago

Yo dawgs, :dog:

Now that you all have a short time for a breather after project 4 is in, take a look at this massively complicated diff and see if it's something we can merge.

Here we go!

kevana commented 11 years ago

At first glance the logic looks good to me, but I haven't dug into everything yet.

taylortrimble commented 11 years ago

I'm getting trigger happy! Someone stop me! #fingeronmergebutton

CrasherCourse commented 11 years ago

The implementation looks fine to me.

However, the pragma section did change a little after your branch off the master and I don't remember how github will deal with it.

Line 87 has a repeat of ReadDMXStartChannel(void); I'm not sure if that is to declare first then define later or not.

Also, can we use half words in c? It might cut down on compare time since 512 needs only 2 bytes instead of 4.

pound holdyourhorses /pound

edit: half words == short

justremembered #poundsarefun

taylortrimble commented 11 years ago

Haha, thanks @CrasherCourse!

Yes, git should take care of nice pretty merging for us unless there is a conflict (which it appears I will have to deal with some time) and I was declaring ReadDMXStartChannel. We'll have to do that for the rest of them sometime!

I'll look into the short type! That's a really good suggestion.

taylortrimble commented 11 years ago

Section 5.4.2 Integer Data Types of the xc8 compiler guide tells us that int and short int are both 16 bits already, so we should be good on fast comparisons!

CrasherCourse commented 11 years ago

Cool! Then, there's nothing obvious that I can tell is wrong.

However, I would remove the now defunct CCP2MX pragma that you commented out.

Strangely, the generic integer Wikipedia entry regards int as 4 bytes and it says newer 32+ bit OS's regard int as a full words in C/C++. But, both the xc8 and c18 compiler use int as shorts so no worries on that.

taylortrimble commented 11 years ago

All right! I merged in master, and I think this is ready to go. Since:

No one hates this refactor so far
It compiles with a smile on its face
It doesn't seem to have shit logic

I'll probably merge this tomorrow if nothing else comes up!

merginga23dayoldpullrequestyay :smile:

kevana commented 11 years ago

Thumbs up On Dec 16, 2012 11:06 PM, "Taylor Trimble" notifications@github.com wrote:

All right! I merged in master, and I think this is ready to go. Since:

No one hates this refactor so far

It compiles with a smile on its face

It doesn't seem to have shit logic

I'll probably merge this tomorrow if nothing else comes up!

merginga23dayoldpullrequestyay [image: :smile:]

— Reply to this email directly or view it on GitHubhttps://github.com/teslaworksumn/HeadMaster/pull/38#issuecomment-11430635.

taylortrimble commented 11 years ago

Cool, why wait then? :shipit: Thanks all!