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.

Sign up for GitHub

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

Jump to bottom

samples: zigbee: add FEM support #3688

Merged
merged 1 commit into from Jan 26, 2021
Merged

samples: zigbee: add FEM support #3688

merged 1 commit into from Jan 26, 2021

Conversation

wbober
Copy link
Contributor

@wbober wbober commented Jan 19, 2021

This adds support for FEM in Zigbee examples. This is mainly docs
updates plus a few overlay files for nRF21540.

Signed-off-by: Wojciech Bober wojciech.bober@nordicsemi.no

@wbober wbober added DNM doc-required PR must not be merged without tech writer approval. labels Jan 19, 2021
@wbober wbober removed the DNM label Jan 20, 2021
@wbober
Copy link
Contributor Author

wbober commented Jan 20, 2021

Thanks @greg-fer . Looks good.

@greg-fer greg-fer requested a review from ru-fu January 21, 2021 08:32
edmont
edmont approved these changes Jan 21, 2021
View reviewed changes
Copy link
Contributor

@edmont edmont left a comment

Choose a reason for hiding this comment

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

LGTM.

Just a nit, some of the copyright headers for the newly created files should say 2021 instead of 2020.

@wbober
Copy link
Contributor Author

wbober commented Jan 21, 2021

@greg-fer I've added a note that FEM can't be combined with multiprotocol yet. Please take a look.

edmont
edmont reviewed Jan 22, 2021
View reviewed changes
samples/zigbee/light_bulb/boards/nrf21540dk_nrf52840.overlay Outdated
@@ -0,0 +1,11 @@
/*
* Copyright (c) 2020 Nordic Semiconductor ASA
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Copyright (c) 2020 Nordic Semiconductor ASA
* Copyright (c) 2021 Nordic Semiconductor ASA

samples/zigbee/light_switch/boards/nrf21540dk_nrf52840.overlay Outdated
@@ -0,0 +1,11 @@
/*
* Copyright (c) 2020 Nordic Semiconductor ASA
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Copyright (c) 2020 Nordic Semiconductor ASA
* Copyright (c) 2021 Nordic Semiconductor ASA

samples/zigbee/network_coordinator/boards/nrf21540dk_nrf52840.overlay Outdated
@@ -0,0 +1,11 @@
/*
* Copyright (c) 2020 Nordic Semiconductor ASA
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Copyright (c) 2020 Nordic Semiconductor ASA
* Copyright (c) 2021 Nordic Semiconductor ASA

@utzig
Copy link
Contributor

utzig commented Jan 22, 2021

Regarding the CI failures, the gist of it is that you're adding an implicit link, "FEM support", into every file (through rst_epilog) and for every file that does not have a section named "FEM support" in it, there will be a warning.

But why doesn't it happen with Sphinx releases <3.3.0? This changed with the merge of sphinx-doc/sphinx#8183, just after 3.2.1 release, which removed the SubstitutionDefinitionsRemover post transform, that was removing broken links from substitutions. This post-transform extension was initially added to solve issues with the Latex builder, and had the side-effect of allowing those links that don't resolve to work.

How to fix it? Easier way with implicit links would be to just copy/paste and not use substitutions, which is what I would suggest for this PR. Another option would be to add back the SubstitutionDefinitionsRemover, renamed to avoid conflict for people with Sphinx<3.3.0. And even another option would be to properly fix the dangling reference checks only for substitutions that are really being used in a given document, but I have no idea how hard that would be.

@ru-fu
Copy link

ru-fu commented Jan 25, 2021

How to fix it? Easier way with implicit links would be to just copy/paste and not use substitutions, which is what I would suggest for this PR.

Based on your explanation, it should also work to put the sentence into a file in doc/includes/ and include the content from there - since it then won't be included in other files that don't have the respective section.

@NordicBuilder
Copy link
Contributor

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

greg-fer
greg-fer requested changes Jan 25, 2021
View reviewed changes
doc/nrf/includes/sample_fem_support.txt Outdated Show resolved Hide resolved
greg-fer
greg-fer requested changes Jan 25, 2021
View reviewed changes
samples/zigbee/light_bulb/README.rst Outdated Show resolved Hide resolved
@wbober
samples: zigbee: add FEM support
1987a5d 
This adds support for FEM in Zigbee examples. This is mainly docs
updates plus a few overlay files for nRF21540.
Edited FEM section in Thread samples to match Zigbee.
Replaced tag-based resure sections with an include tag.

Signed-off-by: Wojciech Bober <wojciech.bober@nordicsemi.no>
@wbober wbober requested a review from greg-fer January 26, 2021 09:46
@rlubos rlubos merged commit ccad284 into nrfconnect:master Jan 26, 2021
@peknis peknis mentioned this pull request Jan 27, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edmont edmont edmont approved these changes

@tomchy tomchy tomchy approved these changes

@ru-fu ru-fu ru-fu approved these changes

@greg-fer greg-fer greg-fer approved these changes

@jangalda-nsc jangalda-nsc jangalda-nsc approved these changes

@carlescufi carlescufi Awaiting requested review from carlescufi

@maciekfabia maciekfabia Awaiting requested review from maciekfabia

@mariuszpos mariuszpos Awaiting requested review from mariuszpos

@lmaciejonczyk lmaciejonczyk Awaiting requested review from lmaciejonczyk

@MarekPorwisz MarekPorwisz Awaiting requested review from MarekPorwisz

Assignees
No one assigned
Labels
doc-required PR must not be merged without tech writer approval.
Projects
None yet
Milestone
No milestone
9 participants
@wbober @utzig @ru-fu @NordicBuilder @edmont @tomchy @greg-fer @jangalda-nsc @rlubos