Skip to content

nrf: Rewrite the DigitalInOut hal using nRFx#1002

Merged
tannewt merged 1 commit into
adafruit:masterfrom
arturo182:nrfx_gpio
Jul 10, 2018
Merged

nrf: Rewrite the DigitalInOut hal using nRFx#1002
tannewt merged 1 commit into
adafruit:masterfrom
arturo182:nrfx_gpio

Conversation

@arturo182
Copy link
Copy Markdown
Collaborator

No description provided.

@arturo182 arturo182 requested a review from tannewt July 9, 2018 04:44
Copy link
Copy Markdown
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Way cleaner. Thank you!

@tannewt tannewt merged commit e875f4e into adafruit:master Jul 10, 2018
@arturo182 arturo182 deleted the nrfx_gpio branch July 10, 2018 05:45
Copy link
Copy Markdown
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

These are followup comments to an already approved PR.

if (self->open_drain) {
digitalio_digitalinout_obj_t *self) {
if (self->open_drain)
return DRIVE_MODE_OPEN_DRAIN;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

same here, etc.

// True is implemented differently between modes so reset the value to make
// sure its correct for the new mode.
if (value) {
if (value)
Copy link
Copy Markdown
Collaborator

@dhalbert dhalbert Jul 10, 2018

Choose a reason for hiding this comment

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

I know this is already approved, but I would ask you not to remove braces around single statements in control structures. Please always use braces in control structures, except perhaps in the most extreme cases (like very stylized single-line code). I've had to fix several bugs over the years that were caused by edits to code with missing braces.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, I'll try to remember, at this point it's muscle memory 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants