Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up unused variables from HardwareSerial.cpp #1769

Merged
merged 1 commit into from Jan 29, 2014

Conversation

3 participants
@ribbons
Copy link

commented Dec 27, 2013

This pull request cleans up a couple of unused variables in HardwareSerial.cpp to improve clarity.

@matthijskooijman

This comment has been minimized.

Copy link
Collaborator

commented Dec 27, 2013

This commit is unfortunately wrong - the result of these variables isn't used, but the side effect of reading the UDR register is that the byte is removed from the FIFO, which is needed. If this line is removed, and a buffer flow occurs, I think that the RX complete interrupt will then continue to occur indefinately...

Assuming that your goal is to silence the compilers warning, you might need to find some other way to keep the read, but get rid of the warning...

Regarding the other variable you are removing, I already have a commit for that in #1711, which will hopefully be merged soon.

@cmaglie

This comment has been minimized.

Copy link
Member

commented Dec 27, 2013

Writing just

UDR0;

instead of

unsigned char c = UDR0;

should do the trick.

@matthijskooijman
I've your serial patch in my queue, I'll review it soon.

C

@ribbons

This comment has been minimized.

Copy link
Author

commented Dec 27, 2013

Oops! Thanks to both of you for your comments. I'll update this tomorrow to take out the current_config removal handled in #1711 and to update the c removal to use the solution proposed by @cmaglie.

@matthijskooijman

This comment has been minimized.

Copy link
Collaborator

commented Dec 27, 2013

@ribbons, if you do that, best base your changes on top of my #1711, which also touches related code.

@ribbons

This comment has been minimized.

Copy link
Author

commented Dec 28, 2013

Commit updated as promised.

@matthijskooijman - Ah yes, I see what you mean now I've looked more closely at your changes. However (being fairly new to this) I'm not sure what is the correct way to achieve this - should I merge the commits from your PR into this branch?

@matthijskooijman

This comment has been minimized.

Copy link
Collaborator

commented Dec 31, 2013

It's probably best if you put your change(s) on top of my change, instead of merging. To do so, you should fetch my changes and create a local branch containing them:

git fetch https://github.com/arduino/Arduino.git refs/pull/1711/head
git checkout -b new_branch FETCH_HEAD

This puts you on a local branch called "new_branch", containing my changes. Now, add any changes you want (you could cherry-pick them, but IIRC I moved code so much that you'll probably want to redo your change from scratch).

When making a pull-request, it seems you can even tell github to use "refs/pull/1711/head" (i.e. my pullreq) as a base (you'll have to manually enter that string, it's not offered in the list). Not sure what this means for the way the pullrequest is merged (I don't think github can know where the pullrequest should be merged to), but that's something to sort out later (the devs can always just manually pull the commit).

@ribbons

This comment has been minimized.

Copy link
Author

commented Jan 1, 2014

Alternatively, shall I wait until #1711 is merged and then re-implement my changes on top of that to save having to open a new PR?

@ribbons

This comment has been minimized.

Copy link
Author

commented Jan 28, 2014

As proposed in my previous comment, I've updated this to remove the unused c variable (now in HardwareSerial_private.h) now that #1711 has been merged.

@matthijskooijman

This comment has been minimized.

Copy link
Collaborator

commented Jan 28, 2014

Did you happen to compare the hex or disassembly output with and without this change? I'd expect them to be equal, but it would be good to doublecheck.

@ribbons

This comment has been minimized.

Copy link
Author

commented Jan 28, 2014

@matthijskooijman Good thinking - I'll do that tomorrow evening and report back...

@cmaglie cmaglie merged commit cd9657f into arduino:ide-1.5.x Jan 29, 2014

@cmaglie

This comment has been minimized.

Copy link
Member

commented Jan 29, 2014

Just checked, the output is exactly the same. Thanks!

@ribbons

This comment has been minimized.

Copy link
Author

commented Jan 29, 2014

Many thanks both for your help - much appreciated.

@ribbons ribbons deleted the ribbons:hardwareserial-unused-vars branch Jan 29, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.