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

docs(feat): further generalize RGB information #2485

Merged
merged 3 commits into from
Nov 2, 2024

Conversation

lesshonor
Copy link
Contributor

@lesshonor lesshonor commented Sep 17, 2024

Using PIO SPI for the "Other Boards" example:

  1. Does a better job of demonstrating how the RGB config for hardware X can differ from hardware Y (the current examples are nearly-identical).
  2. Provides a reference configuration which an increasing number of hobbyist keyboard makers will inevitably end up looking for in the near term. (When/if ZMK moves to Zephyr 3.7, switching to worldsemi,ws2812-pio might be a good idea for further emphasis on point 1.)

Etc.

  • Since the primary motivation is to expand beyond a single hardware configuration, much of the rest of the page was edited with TMTOWTDI in mind.
  • That said, I don't know if any ZMKers have ever tested any of the other LED ICs in the list. Rather than affirmatively stating "ZMK supports X", it's probably better to gently guide users toward checking Zephyr.
  • The adjective "per-key" is more widely-used than "under-key".
  • Reduce duplication. Instead of copying the CONFIG_* section, write it once for end-users then "yes, and" with an example of Kconfig.defconfig defaults for board/shield writers.
  • Explain with examples. Linking to two of Kyria's different board configurations shines more light on the board/shield paradigm. I'm a little less sold on the Corne LED counts but it's accurate at this moment in time.
  • Having an info box and a note box right next to each other deemphasises the "callout" aspect of both and looks a little unsightly, so I moved the low-frequency I/O pin disclaimer to a sentence near the beginning of that section.

What Could Definitely Use Improvement

Maybe the hardware-specific implementations could be a set of tabs for nRF52 WS2812 SPI and RP2040 WS2812 PIO SPI; that would make it easier to pull out the common led_strip definition vs replicating.

The "Other Boards" description is genuinely an awful cop-out, but I'm not sure how to more delicately express that if you can't find an explanation or example of something in ZMK, you should check Zephyr. It doesn't help that Zephyr doesn't appear to have an "led_strip" index; just the sample code, which I'm not sold on linking to. Maybe it'd be better to link to worldsemi, but consider the first bullet in the et cetera list...

@lesshonor lesshonor requested a review from a team as a code owner September 17, 2024 04:33
@caksoylar caksoylar added the documentation Improvements or additions to documentation label Sep 18, 2024
@caksoylar caksoylar self-assigned this Sep 18, 2024
Copy link
Contributor

@Nick-Munnich Nick-Munnich left a comment

Choose a reason for hiding this comment

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

Overall this looks like positive changes to me, just some minor nits. That said, I think there's room for some further improvements to the documentation of lighting:

Like @caksoylar suggested, I think it would make sense to move the adding bits into the "Hardware integration" section (likely leaving a link in the lighting pages when doing so). I think after that is done, the pages would be short enough that it would make sense to merge the backlight and underglow pages into a "Lighting" (or similarly named) page, which could better discuss the differences between the two systems. The adding lighting stuff could also be merged under the hardware integration section.

Would making those changes (or similar) be something you'd be interested in doing/including in this PR?

Comment on lines 41 to 53
### Modifying the Number of LEDs

A common issue when enabling underglow is that some of the installed LEDs do not illuminate. This can happen when a board's default underglow configuration accounts only for either the downward facing LEDs or the upward facing LEDs under each key. On a split keyboard, a good sign that this may be the problem is that the unilluminated LEDs on each half are symmetrical.
If some of the LEDs on your keyboard don't light up, that could be a hardware problem. But another possibility is that the number of LEDs in the default configuration doesn't match the number that are installed. For example, the `corne` shield specifies only 10 LEDs per side while you might have as many as 27. On a split keyboard, a good sign of this problem is if the lit LEDs on each half are symmetrical.

The number of underglow LEDs is controlled by the `chain-length` property in the `led_strip` node. You can [change the value of this property](../config/index.md#changing-devicetree-properties) in the `<keyboard>.keymap` file by adding a stanza like this one outside of any other node (i.e. above or below the `/` node):
The `chain-length` property of the `led_strip` node controls the number of underglow LEDs. If it is incorrect for your build, [you can change this property](../config/index.md#changing-devicetree-properties) in your `<keyboard>.keymap` file by adding a stanza like this one outside of any other node (i.e. above or below the `/` node):

```dts
&led_strip {
chain-length = <21>;
};
```

where the value is the total count of LEDs (per half, for split keyboards).
For split keyboards, set `chain-length` to the number of LEDs installed on each half.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not entirely happy with the subsection name considering the text. It makes sense to make a note of this somewhere for cases like the one described, but the name doesn't seem "troubleshooting" enough for my taste. It may also make sense to move this out into a "feature troubleshooting" section at some point.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe change it to something like "Troubleshooting Partially Working LEDs" (a mouthful, so maybe not the best) for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This section has zero meaningful troubleshooting information, so why not remove any mention of "problems" altogether? Aside from making the section shorter and more direct, IMO it fits in better as a subheading of enabling RGB underglow this way; technically neglecting to enable the Kconfig seting in the first place is a problem that may need to be "troubleshot", but we don't call it one here. Same can go for changing the number of LEDs.


### nRF52-Based Boards

With nRF52 boards, you can just use `&spi3` and define the pins you want to use.
Using an SPI-based LED strip driver on the `&spi3` interface is the simplest option for nRF52-based boards. If possible, avoid using pins which are limited to low-frequency I/O for this purpose. The resulting interference may result in poor wireless performance.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'd move the warning back into the info box and turn the info box into a warning box, personally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something like...

-Using an SPI-based LED strip driver on the `&spi3` interface is the simplest option for nRF52-based boards. If possible, avoid using pins which are limited to low-frequency I/O for this purpose. The resulting interference may result in poor wireless performance.
+Using an SPI-based LED strip driver on the `&spi3` interface is the simplest option for nRF52-based boards.
 
-:::info
+:::warning
 
-The list of low frequency I/O pins for the nRF52840 can be found [here](https://docs.nordicsemi.com/bundle/ps_nrf52840/page/pin.html).
+Avoid using pins which are limited to low-frequency I/O for this purpose, as the resulting interference may result in poor wireless performance. [See Nordic's documentation](https://docs.nordicsemi.com/bundle/ps_nrf52840/page/pin.html) for pin assignments.

...isn't immediately terrible, but having more text in the callout box than outside of it strikes me as the docs equivalent of a code smell.

Some other thoughts:

  • Is there a quantifiable measurement of how badly performance is degraded which could support a :::warning?
    • This may be because a.) the most popular designs use high-frequency pins for WS2812 DI, and b.) interference problems are notoriously difficult to root cause—but anecdotally, we've all encountered far more complaints about RGB drastically curtailing battery life than RGB interfering with BLE. And while in either case the problem can be mitigated with specific hardware design decisions, only the former is explicitly called attention to here.
  • Regardless of impact, only a hardware designer with non-finalized work can take meaningful action on this information. The average reader just gets to deal with it. This situation is what it is...but a non-actionable :::warning isn't great.
  • I believe it would be worth noting RGB as a potential cause of connection issues on the related troubleshooting page.

Copy link
Contributor

@Nick-Munnich Nick-Munnich Oct 20, 2024

Choose a reason for hiding this comment

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

I think the entire warning and much of the section could probably be dropped if we can get #2508 merged, linking to that instead.

I've seen a clip of the SPI nodes on the n!n causing noticeable input lag when used with a cirque, which I am of the (unconfirmed) belief is due to the low-frequency pins. I've not seen an example of RGB, but I think that's because pretty much all shields will just use the pro micro layout's RX or TX pins for that, and designers who stray are usually smart enough to check.

In theory, a user could bodge a connection and adjust the definition. Probably not worth considering, though.

I semi-agree: I think we could note down low-frequency pins being used for high-frequency purposes, and I think it would be better positioned in #2360. EDIT: I have now added such a note to #2360.

docs/docs/features/underglow.md Outdated Show resolved Hide resolved
docs/docs/features/underglow.md Outdated Show resolved Hide resolved
@caksoylar
Copy link
Contributor

Like @caksoylar suggested, I think it would make sense to move the adding bits into the "Hardware integration" section (likely leaving a link in the lighting pages when doing so). I think after that is done, the pages would be short enough that it would make sense to merge the backlight and underglow pages into a "Lighting" (or similarly named) page, which could better discuss the differences between the two systems. The adding lighting stuff could also be merged under the hardware integration section.

Would making those changes (or similar) be something you'd be interested in doing/including in this PR?

I think it might be better to move things to the new "Hardware integration" wholesale in a new PR. But it would be good to keep that in mind while updating this page, so that it can be done as a simple copy/paste.

Copy link
Contributor

@Nick-Munnich Nick-Munnich left a comment

Choose a reason for hiding this comment

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

I'm happy with this as-is for now. Working in the new pinctrl page is the only point I think I have remaining, and I'm happy for that to happen in parallel with reshuffling to move the setup details into hardware configuration.

@Nick-Munnich Nick-Munnich merged commit 6e37f21 into zmkfirmware:main Nov 2, 2024
7 checks passed
EnotionZ pushed a commit to EnotionZ/zmk that referenced this pull request Nov 6, 2024
* docs(feat): Provide example of PIO SPI for RGB underglow

* docs(feat): further generalize RGB information

* docs: use nice_nano_v2 for board-specific shield config example

Co-authored-by: Cem Aksoylar <[email protected]>

---------

Co-authored-by: Cem Aksoylar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants