Skip to content

drivers/enc28j60/enc28j60.c: fix counter var length#7857

Merged
kYc0o merged 1 commit intoRIOT-OS:masterfrom
kYc0o:fix_enc28j60_comp
Nov 7, 2017
Merged

drivers/enc28j60/enc28j60.c: fix counter var length#7857
kYc0o merged 1 commit intoRIOT-OS:masterfrom
kYc0o:fix_enc28j60_comp

Conversation

@kYc0o
Copy link
Copy Markdown
Contributor

@kYc0o kYc0o commented Oct 26, 2017

While compiling tests/driver_enc28j60 with clang in OS X it discovers several minor issues.

This PR fixes them.

@kYc0o kYc0o added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: drivers Area: Device drivers Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 26, 2017
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine except for one comment.

{
enc28j60_t *dev = (enc28j60_t *)netdev;
int res;
unsigned res;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Res is used uninitialized on L326. I think it is missing a res = in front of the spi_init_cs function call

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, fixed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, spi_init_cs returns an int, and the printf on L324/L325 uses %i as well. Was the original error message about a signed vs unsigned comparison on L337?
Removing the U from the definition of STARTUP_TIMEOUT would probably fix the comparison.
Another alternative would be to introduce a new variable unsigned count = 0 on line L334 instead. The compiler will see that res is unused after L325 and release the register/memory used by it. Also, the declaration of res could move to L323.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I agree making STARTUP_TIMEOUT an int would be better than continuing to create more problems.

@kYc0o kYc0o mentioned this pull request Oct 26, 2017
16 tasks
@kYc0o
Copy link
Copy Markdown
Contributor Author

kYc0o commented Oct 31, 2017

@gebart I have addressed your comment, is this ready?

Copy link
Copy Markdown
Member

@jnohlgard jnohlgard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

@kYc0o kYc0o force-pushed the fix_enc28j60_comp branch from f3db9dd to 58e3fab Compare November 7, 2017 10:46
@kYc0o
Copy link
Copy Markdown
Contributor Author

kYc0o commented Nov 7, 2017

Squashed.

@kYc0o kYc0o merged commit a423897 into RIOT-OS:master Nov 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants