AVR GPIO macro defines more readable

[mtei]

Description

• A little easier to read the definition of the GPIO control macro for AVR.
  quantum/quantum.h, quantum/config_common.h
• Modified split_common/serial.c to use qmk_firmware standard GPIO control macro.
  quantum/split_common/serial.c

There is no change in compilation results for this PR.

Types of Changes

• Core
• Bugfix
• New feature
• Enhancement/optimization
• Keyboard (addition or update)
• Keymap/layout/userspace (addition or update)
• Documentation

Checklist

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• I have tested the changes and verified that they work and don't break anything (as well as I can manage).

[drashna]

This looks good to me.

@pelrun ?

[pelrun]

The duplicated definitions in serial.c bug me a little bit, but not enough to change.

[mtei]

I deliberately avoided serial.c to depend on quantum.h. serial.c is a very timing-dependent module, so we want to avoid other impacts as much as possible.

[drashna]

In that case, would it make sense to move both out of the respective files, and into a separate header file, where both quantum.h and serial.c could include them?

That way, we're not maintaining multiple copies of the GPIO commands, and don't have to worry about keeping them in sync (for behavioral reasons)?

[mtei]

In that case, would it make sense to move both out of the respective files, and into a separate header file, where both quantum.h and serial.c could include them?

I already thought about that method, but I did not adopt it. If serial.c imports gpio_control.h, you need to be careful modifying gpio_control.h as you would for serial.c. It's not desirable.

That way, we're not maintaining multiple copies of the GPIO commands, and don't have to worry about keeping them in sync (for behavioral reasons)?

Do you need to sync? I do not think that it is necessary. It is dangerous to change the working serial.c just to synchronize.

[fauxpark]

If serial.c imports gpio_control.h, you need to be careful modifying gpio_control.h as you would for serial.c.

Not sure I understand. Of course you need to be careful modifying these GPIO macros, however they are fairly simple so I would be surprised if there were any major problems with them that haven't been discovered yet. If something does need to be changed, they must both be changed anyway, because otherwise you have two conflicting #defines which will probably make GCC complain.

The thing is that serial.c shouldn't need to know nor define how eg. setPinOutput() works at this level. All of that should be abstracted away, that's the whole point of these macros in the first place.

[mtei]

In general I like to be abstract. However, the abstraction is not necessary only for serial.c. What is important is that the generated binaries do not change.

We have already experienced that when the GCC version changes the generation code will change and the timing changes. (#4269) We can't trust the simplicity of the source code's appearance.

[drashna]

It looks like there is a merge conflict here, now.

[mtei]

Thanks, I fixed.

Then I checked again that there was no change in the binary generated by this PR.

$ git checkout avr_gpio_macro_more_readable
Switched to branch 'avr_gpio_macro_more_readable'
$ make SKIP_DEBUG_INFO=y SKIP_VERSION=y lets_split:default:clean
$ make SKIP_DEBUG_INFO=y SKIP_VERSION=y lets_split:default
....
 * The firmware size is fine - 19450/28672 (67%, 9222 bytes free)
$ mv lets_split_rev2_default.hex new-lets_split_rev2_default.hex

$ git checkout master
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
$ make SKIP_DEBUG_INFO=y SKIP_VERSION=y lets_split:default:clean
$ make SKIP_DEBUG_INFO=y SKIP_VERSION=y lets_split:default
....
 * The firmware size is fine - 19450/28672 (67%, 9222 bytes free)

$ md5 *lets_split_rev2_default.hex
MD5 (lets_split_rev2_default.hex) = 4999375ced50a8e7f731bc06bdeb534f
MD5 (new-lets_split_rev2_default.hex) = 4999375ced50a8e7f731bc06bdeb534f

[drashna]

Looks good!