Skip to content

drivers/sdcard_spi#7853

Closed
kYc0o wants to merge 2 commits intoRIOT-OS:masterfrom
kYc0o:fix_sdcard_misc
Closed

drivers/sdcard_spi#7853
kYc0o wants to merge 2 commits intoRIOT-OS:masterfrom
kYc0o:fix_sdcard_misc

Conversation

@kYc0o
Copy link
Copy Markdown
Contributor

@kYc0o kYc0o commented Oct 26, 2017

While compiling tests/driver_sdcard_spi 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 Area: tests Area: tests and testing framework labels Oct 26, 2017
jnohlgard
jnohlgard requested changes Oct 26, 2017
View reviewed changes
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, one minor comment.

drivers/sdcard_spi/sdcard_spi.c
do {
if (_dyn_spi_rxtx_byte(card, SD_CARD_DUMMY_BYTE, &read_byte) == 1) {
if (read_byte == 0xFF) {
if (read_byte == (char)0xFF) {
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard Oct 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think it is better to change the type of read_byte to uint8_t, here and in the other read_byte variables as well

Copy link
Copy Markdown
Contributor Author

@kYc0o kYc0o Oct 26, 2017

Choose a reason for hiding this comment

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

Yes I prefer that too. Will change.

@kYc0o
Copy link
Copy Markdown
Contributor Author

kYc0o commented Oct 26, 2017

While fixing @gebart comment I found an extensive use of char* in the code to specify buffers. AFAIR @miri64 has already pointed out this to get a consensus on which is better to use, either char* or uint8_t*, and the consensus was to use uint8_t* if I'm not wrong. Therefore, for this file I guess I should modify all the usages of char* to uint8_t*. However, I don't know if that change is in the scope of this PR.

I don't mind to do it here, just want opinions.

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

MichelRottleuthner commented Oct 27, 2017

I'd vote for simply adding the char to uint8_t fixes to this PR as a lot of the changes you introduced here are for fixing datatypes anyway.

@kYc0o
Copy link
Copy Markdown
Contributor Author

kYc0o commented Oct 29, 2017

So @gebart what's your opinion on this?

@jnohlgard
Copy link
Copy Markdown
Member

jnohlgard commented Oct 30, 2017

@kYc0o I'm OK if you want to change the buffer pointer types from char to uint8_t (for this driver and test only) in this PR.
You need to set a better title for the PR.

@PeterKietzmann
Copy link
Copy Markdown
Member

PeterKietzmann commented Nov 16, 2017

What is this PR waiting for?

@kYc0o
Copy link
Copy Markdown
Contributor Author

kYc0o commented Nov 17, 2017

My actions, I have other things in the queue but I'll work on this ASAP.

@miri64 miri64 added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Nov 17, 2017
@kYc0o kYc0o added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 28, 2017
@kYc0o
Copy link
Copy Markdown
Contributor Author

kYc0o commented Nov 28, 2017

Well apparently this was fixed by #7919 but following another approach. Should we close this and leave it as is or a more clean solutions should be pursued here?

@PeterKietzmann
Copy link
Copy Markdown
Member

PeterKietzmann commented Jan 5, 2018

@kYc0o it seems there are no strong opinions to have this solution in. I'll close this one now in favor of #7919. New PRs can always be opened, as usual...

@MichelRottleuthner MichelRottleuthner mentioned this pull request Feb 26, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jnohlgard jnohlgard jnohlgard requested changes

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

Assignees

@MichelRottleuthner MichelRottleuthner

Labels

Area: drivers Area: Device drivers Area: tests Area: tests and testing framework 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 State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@kYc0o @MichelRottleuthner @jnohlgard @PeterKietzmann @miri64