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

drivers/ir_nec: NEC remote receiver implementation #17935

Merged
merged 2 commits into from
Jun 1, 2022
Merged

Conversation

dp1
Copy link
Contributor

@dp1 dp1 commented Apr 13, 2022

Contribution description

This PR adds support for decoding packets sent by infrared remotes using the NEC protocol. I tested this code with an HS0038B receiver, which outputs a demodulated and inverted signal (the inversion seems to be quite common, but I haven't done an extensive search on this).

Testing procedure

Board setup

Connect +3v3 and GND to the chip, and its output pin (which needs a capacitor and 1/2 resistors, see page 2 here ) to an interrupt-capable gpio input. I am using this module and a b-l475e-iot01a board

Code

The following receives and prints decoded packets

#include "ir_nec.h"

ir_nec_t remote;
ir_nec_cmd_t cmd;
ir_nec_params_t ir_params = {.pin = GPIO_PIN(PORT_A, 4)};

int main(void) {
    ir_nec_init(&remote, &ir_params);

    for(;;) {
        if(ir_nec_read(&remote, &cmd)) {
            puts("error");
            return -1;
        }
        printf("Packet addr=0x%X, cmd=0x%X\n", cmd.addr, cmd.cmd);
    }
}

Issues/PRs references

See #17906 for the initial issue on this feature

@github-actions github-actions bot added Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration labels Apr 13, 2022
@dp1 dp1 changed the title drivers/ir_nec: ir remote implementation drivers/ir_nec: NEC remote implementation Apr 13, 2022
@dp1 dp1 changed the title drivers/ir_nec: NEC remote implementation drivers/ir_nec: NEC remote receiver implementation Apr 13, 2022
@github-actions github-actions bot added the Area: tools Area: Supplementary tools label Apr 15, 2022
@fjmolinas fjmolinas requested a review from chrysn April 20, 2022 16:15
@benpicco benpicco requested a review from kfessel May 23, 2022 22:02
drivers/ir_nec/ir_nec.c Outdated Show resolved Hide resolved
drivers/include/ir_nec.h Outdated Show resolved Hide resolved
drivers/include/ir_nec.h Outdated Show resolved Hide resolved
@kfessel
Copy link
Contributor

kfessel commented May 24, 2022

please provide test/driver_ir_nec

@github-actions github-actions bot added Area: doc Area: Documentation Area: tests Area: tests and testing framework labels May 24, 2022
@dp1
Copy link
Contributor Author

dp1 commented May 24, 2022

Thank you for the feedback, I just added a few commits that should address all the issues you mentioned. Please let me know if there's anything more I should change

Copy link
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

I still would prefer this using ztimer.

other than that there are these nitpicks.
the PR tested succsessfully

tests/driver_ir_nec/main.c Show resolved Hide resolved
tests/driver_ir_nec/main.c Outdated Show resolved Hide resolved
tests/driver_ir_nec/main.c Outdated Show resolved Hide resolved
tests/driver_ir_nec/Makefile Show resolved Hide resolved
drivers/ir_nec/include/ir_nec_constants.h Outdated Show resolved Hide resolved
@dp1
Copy link
Contributor Author

dp1 commented May 25, 2022

It should be good now, I applied the changes you suggested and moved from xtimer to ztimer. I also tested again on the hardware to make sure that everything works and found no problems in doing so.

@kfessel kfessel added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 27, 2022
@kfessel
Copy link
Contributor

kfessel commented May 27, 2022

looks good to go, please squash

@dp1
Copy link
Contributor Author

dp1 commented May 27, 2022

I squashed the changes down to 3 commits, trying to keep different areas (drivers/ and tests/) in separate commits - do you want me to squash everything in a single one?

@kfessel
Copy link
Contributor

kfessel commented May 27, 2022

lets wait for murdock

Copy link
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

sadly Murdock seems not to like specific defaults therefor: Use GPIO_PIN(0,0) for fallback

tests/driver_ir_nec/Makefile Outdated Show resolved Hide resolved
@kfessel
Copy link
Contributor

kfessel commented May 30, 2022

it ok if you squash this change right into the: tests/driver_ir_nec: add test application for ir_nec driver commit

@dp1
Copy link
Contributor Author

dp1 commented May 30, 2022

Thank you for the help! It should be good now, let's see what Murdock thinks of it

Copy link
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

one minor change
and
please move the documentation change in ir_nec_constants.h from the test commit to the driver commit

drivers/ir_nec/ir_nec.c Outdated Show resolved Hide resolved
drivers/ir_nec/ir_nec.c Outdated Show resolved Hide resolved
Copy link
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

works, reads well, tests well

@kfessel kfessel added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels May 31, 2022
@kfessel kfessel merged commit 6e41c68 into RIOT-OS:master Jun 1, 2022
@dp1 dp1 deleted the ir_nec branch June 1, 2022 13:53
@chrysn chrysn added this to the Release 2022.07 milestone Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: doc Area: Documentation Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: tests Area: tests and testing framework Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants