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

Audio_Enable, AVR template update #8901

Merged
merged 4 commits into from
May 1, 2020

Conversation

JohSchneider
Copy link
Contributor

@JohSchneider JohSchneider commented Apr 24, 2020

quantum/audio/audio_avr.c does not implicitly enable any default pins used as output - unless the correct define is set
the comment automatically generated from quantum/template/avr/rules.mk is hence misleading

Description

if a user enables audio and forgets to define a pin on AVR based projext, they'll end up with the audio-feature not working, wasting space/bytes in the firmware and - worst case - breaking the build because the firmware size exceeds the maximum

these patches address this by correcting the comment in the avr template,

Types of Changes

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

Issues Fixed or Closed by This PR

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).

@JohSchneider JohSchneider force-pushed the audio_enable_corrections branch from e9c539d to be309c3 Compare April 24, 2020 14:33
@zvecr
Copy link
Member

zvecr commented Apr 24, 2020

Chances of 800+ files getting reviewed is super slim, and generally we ask for PRs to be broken into chunks (50-100 depending on change).

Also, I would rather drive this via #6165 than preempt changes, lets get it to a solid point and then work out what we want to do.

@JohSchneider
Copy link
Contributor Author

thanks for the feedback - the changes do 90% of the time the same thing... i can break off the ones which do exactly the same if that helps?

@JohSchneider JohSchneider force-pushed the audio_enable_corrections branch from be309c3 to 6ffbcf5 Compare April 24, 2020 16:08
@JohSchneider
Copy link
Contributor Author

there - this should be better (?)

@zvecr
Copy link
Member

zvecr commented Apr 24, 2020

Lets get the wording approved here before raising all the extra PRs, given its just adding rework for you.

@zvecr zvecr mentioned this pull request Apr 24, 2020
13 tasks
@JohSchneider
Copy link
Contributor Author

okay - that's reasonable :-)

@JohSchneider
Copy link
Contributor Author

hm, did i make some mistake creating this PR for travis to complain about
"The command "git diff --name-only HEAD $TRAVIS_BRANCH" exited with 128."?

not on its own, it needs a pin configured per define in config.h for audio to actually work
otherwise only parts of the code are included in the firmware, wasting space and possibly breaking builds because auf hitting the firmware-size limits
@JohSchneider JohSchneider force-pushed the audio_enable_corrections branch from 6ffbcf5 to 9ebf490 Compare April 28, 2020 17:03
@drashna drashna requested a review from a team April 29, 2020 05:41
@drashna drashna added the core label Apr 29, 2020
quantum/template/avr/rules.mk Outdated Show resolved Hide resolved
@drashna drashna requested a review from a team April 29, 2020 10:26
@JohSchneider JohSchneider requested review from fauxpark and removed request for a team April 29, 2020 14:11
@JohSchneider
Copy link
Contributor Author

should PR #8904 be reopened and refactored as well? to avoid future misconfigurations on exiting keyboards?

@JohSchneider
Copy link
Contributor Author

@fauxpark : picking up your suggestion of a compile-time error for AVR boards with #8974

@JohSchneider
Copy link
Contributor Author

"and removed request for qmk/collaborators" err... why did that happen when i only requested a re-review for fauxpark?

Copy link
Member

@zvecr zvecr left a comment

Choose a reason for hiding this comment

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

With #8904, again, no we don't want 800 file changes in one PR. They will need to be done in small blocks of maybe 50 to 100.

docs/ChangeLog/20200530/PR8901.md Outdated Show resolved Hide resolved
@JohSchneider JohSchneider force-pushed the audio_enable_corrections branch from 0db53f5 to f7d472d Compare April 29, 2020 16:34
@zvecr zvecr changed the base branch from future to master May 1, 2020 20:28
@zvecr zvecr merged commit ddd055b into qmk:master May 1, 2020
violet-fish pushed a commit to violet-fish/qmk_firmware that referenced this pull request May 3, 2020
* Branch point for 2020 May 30 Breaking Change

* audio-configuration: template: audio_avr.c does NOT default to C6

not on its own, it needs a pin configured per define in config.h for audio to actually work
otherwise only parts of the code are included in the firmware, wasting space and possibly breaking builds because auf hitting the firmware-size limits

* audio-configuration: strip comment to bare essentials

Co-Authored-By: Ryan <[email protected]>

* revert future change

Co-authored-by: James Young <[email protected]>
Co-authored-by: Johannes <[email protected]>
Co-authored-by: Ryan <[email protected]>
Co-authored-by: zvecr <[email protected]>
JohSchneider added a commit to JohSchneider/yamsek.qmk_firmware that referenced this pull request May 7, 2020
noroadsleft pushed a commit that referenced this pull request May 10, 2020
* audio-configuration: fix avr boards with wrong configuration

the audiopin with avr boards does not default to anything, if it is not set as a define in config.h, audio output won't work - even worse the compiler will include parts of the audio-system in the firmware, wasting space and possibly failing builds because of the firmware exceeding the maximum size

* audio-configuration: add Changelog entry

* audio-configuration: fix avr boards with wrong configuration

the audiopin with avr boards does not default to anything, if it is not set as a define in config.h, audio output won't work - even worse the compiler will include parts of the audio-system in the firmware, wasting space and possibly failing builds because of the firmware exceeding the maximum size

* audio-configuration: align comment changes with #8901

* audio-configuration: changelow wording

* audio-configuration: remove rules.mk

since it now would change nothing compared to the keyboards base rule.mk
bitherder pushed a commit to bitherder/qmk_firmware that referenced this pull request May 15, 2020
* Branch point for 2020 May 30 Breaking Change

* audio-configuration: template: audio_avr.c does NOT default to C6

not on its own, it needs a pin configured per define in config.h for audio to actually work
otherwise only parts of the code are included in the firmware, wasting space and possibly breaking builds because auf hitting the firmware-size limits

* audio-configuration: strip comment to bare essentials

Co-Authored-By: Ryan <[email protected]>

* revert future change

Co-authored-by: James Young <[email protected]>
Co-authored-by: Johannes <[email protected]>
Co-authored-by: Ryan <[email protected]>
Co-authored-by: zvecr <[email protected]>
noroadsleft pushed a commit that referenced this pull request May 15, 2020
* audio-configuration: fix avr boards with wrong configuration

the audiopin with avr boards does not default to anything, if it is not set as a define in config.h, audio output won't work - even worse the compiler will include parts of the audio-system in the firmware, wasting space and possibly failing builds because of the firmware exceeding the maximum size

* audio-configuration: add Changelog entry

* audio-configuration: fix avr boards with wrong configuration

the audiopin with avr boards does not default to anything, if it is not set as a define in config.h, audio output won't work - even worse the compiler will include parts of the audio-system in the firmware, wasting space and possibly failing builds because of the firmware exceeding the maximum size

* audio-configuration: align comment changes with #8901

* audio-configuration: changelow wording

* audio-configuration: remove rules.mk

since it now would change nothing compared to the keyboards base rule.mk
noroadsleft pushed a commit to noroadsleft/qmk_firmware that referenced this pull request May 22, 2020
* audio-configuration: fix avr boards with wrong configuration

the audiopin with avr boards does not default to anything, if it is not set as a define in config.h, audio output won't work - even worse the compiler will include parts of the audio-system in the firmware, wasting space and possibly failing builds because of the firmware exceeding the maximum size

* audio-configuration: add Changelog entry

* audio-configuration: fix avr boards with wrong configuration

the audiopin with avr boards does not default to anything, if it is not set as a define in config.h, audio output won't work - even worse the compiler will include parts of the audio-system in the firmware, wasting space and possibly failing builds because of the firmware exceeding the maximum size

* audio-configuration: align comment changes with qmk#8901

* audio-configuration: changelow wording

* audio-configuration: remove rules.mk

since it now would change nothing compared to the keyboards base rule.mk
sowbug pushed a commit to sowbug/qmk_firmware that referenced this pull request May 24, 2020
* Branch point for 2020 May 30 Breaking Change

* audio-configuration: template: audio_avr.c does NOT default to C6

not on its own, it needs a pin configured per define in config.h for audio to actually work
otherwise only parts of the code are included in the firmware, wasting space and possibly breaking builds because auf hitting the firmware-size limits

* audio-configuration: strip comment to bare essentials

Co-Authored-By: Ryan <[email protected]>

* revert future change

Co-authored-by: James Young <[email protected]>
Co-authored-by: Johannes <[email protected]>
Co-authored-by: Ryan <[email protected]>
Co-authored-by: zvecr <[email protected]>
noroadsleft pushed a commit to noroadsleft/qmk_firmware that referenced this pull request May 28, 2020
* audio-configuration: fix avr boards with wrong configuration

the audiopin with avr boards does not default to anything, if it is not set as a define in config.h, audio output won't work - even worse the compiler will include parts of the audio-system in the firmware, wasting space and possibly failing builds because of the firmware exceeding the maximum size

* audio-configuration: add Changelog entry

* audio-configuration: fix avr boards with wrong configuration

the audiopin with avr boards does not default to anything, if it is not set as a define in config.h, audio output won't work - even worse the compiler will include parts of the audio-system in the firmware, wasting space and possibly failing builds because of the firmware exceeding the maximum size

* audio-configuration: align comment changes with qmk#8901

* audio-configuration: changelow wording

* audio-configuration: remove rules.mk

since it now would change nothing compared to the keyboards base rule.mk
turky pushed a commit to turky/qmk_firmware that referenced this pull request Jun 13, 2020
* Branch point for 2020 May 30 Breaking Change

* audio-configuration: template: audio_avr.c does NOT default to C6

not on its own, it needs a pin configured per define in config.h for audio to actually work
otherwise only parts of the code are included in the firmware, wasting space and possibly breaking builds because auf hitting the firmware-size limits

* audio-configuration: strip comment to bare essentials

Co-Authored-By: Ryan <[email protected]>

* revert future change

Co-authored-by: James Young <[email protected]>
Co-authored-by: Johannes <[email protected]>
Co-authored-by: Ryan <[email protected]>
Co-authored-by: zvecr <[email protected]>
jakobaa pushed a commit to jakobaa/qmk_firmware that referenced this pull request Jul 7, 2020
* Branch point for 2020 May 30 Breaking Change

* audio-configuration: template: audio_avr.c does NOT default to C6

not on its own, it needs a pin configured per define in config.h for audio to actually work
otherwise only parts of the code are included in the firmware, wasting space and possibly breaking builds because auf hitting the firmware-size limits

* audio-configuration: strip comment to bare essentials

Co-Authored-By: Ryan <[email protected]>

* revert future change

Co-authored-by: James Young <[email protected]>
Co-authored-by: Johannes <[email protected]>
Co-authored-by: Ryan <[email protected]>
Co-authored-by: zvecr <[email protected]>
sjmacneil pushed a commit to sjmacneil/qmk_firmware that referenced this pull request Feb 19, 2021
* Branch point for 2020 May 30 Breaking Change

* audio-configuration: template: audio_avr.c does NOT default to C6

not on its own, it needs a pin configured per define in config.h for audio to actually work
otherwise only parts of the code are included in the firmware, wasting space and possibly breaking builds because auf hitting the firmware-size limits

* audio-configuration: strip comment to bare essentials

Co-Authored-By: Ryan <[email protected]>

* revert future change

Co-authored-by: James Young <[email protected]>
Co-authored-by: Johannes <[email protected]>
Co-authored-by: Ryan <[email protected]>
Co-authored-by: zvecr <[email protected]>
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
* Branch point for 2020 May 30 Breaking Change

* audio-configuration: template: audio_avr.c does NOT default to C6

not on its own, it needs a pin configured per define in config.h for audio to actually work
otherwise only parts of the code are included in the firmware, wasting space and possibly breaking builds because auf hitting the firmware-size limits

* audio-configuration: strip comment to bare essentials

Co-Authored-By: Ryan <[email protected]>

* revert future change

Co-authored-by: James Young <[email protected]>
Co-authored-by: Johannes <[email protected]>
Co-authored-by: Ryan <[email protected]>
Co-authored-by: zvecr <[email protected]>
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.

5 participants