-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat(docs) Adding some additional clarity around low power states #2330
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in general: While touching the soft-off page, do you mind adding a commit to remove label = ..
properties from the examples in that page? That's the only spot they are left in the docs.
Upgraded the firmware on my keyboard recently. Ran into an issue where I couldn't wake it from sleep and couldn't figure out why until I saw this PR. I'm not sure about the soft-off portion of this PR, but I think the comment about |
7ad720f
to
ee50a89
Compare
Thanks again Pete for the reply. I have done some larger changes to accommodate. I think this version reads much nicer. Unfortunately a rename was involved which makes reviewing it more difficult. Essentially, the soft off feature page is renamed to low power states, and the information about idle and deep sleep were moved into it. I also documented how to reuse a key for wakeup-only, and how to make the &soft_off behave like &sleep. Some minor renaming was involved to make things read nicer. All the content for the direct pin approach is tested and working. The content for the matrix-integrated and switch matrix wakeup-only was working, but currently isn't: see #2376. It may need some changing after said issue is resolved. |
Did some reshuffling to try and get the tabs behaving nicely without too much duplication. Since this required shoving a bunch of content into other files, I also did so for some content that doesn't get duplicated because it makes the file overall much easier to read. The wakeup-only approach is much simplified and should be clearer now, with minor changes to the direct and matrix approaches as a result of the reshuffling. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wakeup-only case is much clearer for me with these changes, thanks.
Couple general replacements:
- [Bb]"ehaviour" -> "ehavior" (including filenames?)
- [Kk]"eyswitch" -> "ey switch"
It would also be good to add dts
to the fenced code blocks, since we get syntax highlighting for those.
Made the dts and general replacements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am happy with this, thanks again for tackling it. However I'd appreciate if @petejohanson could go over the finished page to confirm whether
- there are any technical correctness issues I missed
- he has any concerns on documenting the wakeup-only approach
733f215
to
ab03ece
Compare
Did a refactoring of this to make it cleaner (as was discussed previously on Discord). Originally I intended to do the refactor after this is merged, but as it has been slow-going I decided to put in the effort to separate out the content. Should make everything easier to review as well. The "guide" portion is now a separate PR: #2439 I added what information I could to the config section, but it may need double-checking. |
201dc7b
to
6189b07
Compare
Folded the content from #2439 back into this - trying to get it nicely into separate PRs post development reshuffling was annoying me too much. |
All looks reasonable to me. The description of deep sleep is a tad... non-technical, and that may be the intention. If so, looks fine to me. If we want to offer more details on the specifics of the power state of the MCU, RAM retention (or not), etc., I'm not sure if that's best served living on that page or elsewhere. |
This is a good idea. I'll have a go at adding such specifics when I touch this page up a bit again to get it ready for merging. |
3e847ee
to
4bee39c
Compare
…ith guide portion moved to hardware integration Co-authored-by: Cem Aksoylar <[email protected]>
7681a22
to
14fe8e6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only a few small notes, LGTM otherwise.
Co-authored-by: Cem Aksoylar <[email protected]>
…kfirmware#2330) * docs(feat): Adding some additional clarity around low power states, with guide portion moved to hardware integration --------- Co-authored-by: Cem Aksoylar <[email protected]>
…kfirmware#2330) * docs(feat): Adding some additional clarity around low power states, with guide portion moved to hardware integration --------- Co-authored-by: Cem Aksoylar <[email protected]>
Minor improvements to resolve some questions that appeared on Discord. Also made a note of the soft-off/deep-sleep wakeup-sources issue, since I couldn't find that documented anywhere.