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

Jump to bottom

Added some reasonable defaults to LIRC's module #53795

Closed
wants to merge 6 commits into from
Closed

Added some reasonable defaults to LIRC's module #53795

wants to merge 6 commits into from

Conversation

aakropotkin
Copy link
Contributor

@aakropotkin aakropotkin commented Jan 11, 2019 •
edited

Motivation for this change

After spending a few hours trying to install LIRC I realized that many of my issues were from a lack of default settings that could get the program up and running. This will allow many users to let LIRC run out of the box.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • [x ] NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@markuskowa
Copy link
Member

CC @ck3d

Mic92
Mic92 reviewed Jan 11, 2019
View reviewed changes
nixos/modules/services/hardware/lirc.nix Outdated

[lircmd]
uinput = False
nodaemon = False
Copy link
Member

@Mic92 Mic92 Jan 11, 2019 •
edited

Choose a reason for hiding this comment

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

To bad this was not added in the first place. I would prefer having sane default options, that can be extended by extraOptions. Do you think users would need to extend the default options in the common case?

Copy link
Member

Choose a reason for hiding this comment

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

I think for now this is alright, can always add that later

Copy link
Member

Choose a reason for hiding this comment

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

agreed.

@ck3d
Copy link
Contributor

ck3d commented Jan 12, 2019 •
edited

My configuration file looks as follows:

[lircd]
driver = ftdi
device = product=0x6015

I set none of the default values in my configuration file without any problems. Could you please specify which problems you have?

@infinisil
Copy link
Member

@BadDecisionsAlex We should strive to keep the default config as small as possible. If @ck3d's config works then that is preferred.

@aakropotkin
Copy link
Contributor Author

My configuration file looks as follows:

[lircd]
driver = ftdi
device = product=0x6015

I set none of the default values in my configuration file without any problems. Could you please specify which problems you have?

Sorry for the late reply, I've been swamped.
Without the default settings that I added LIRC was unable to access any of the drivers which it is normally packaged with.

@ck3d
Copy link
Contributor

ck3d commented Feb 9, 2019

No problem.
To check that lircd find its drivers, call following command:

lircd -H help

In my case, everything is found. The default, ftdi and 30 more drivers.

I think the main reason is the missing device setting. Please give following config a try:

[lircd]
device = /dev/lirc0

or set it as extraArguments:
extraArguments = [ "--device=/dev/lirc0" ];

I had a look into lirc sources and it showed that the default device of the lircd default driver is /dev/lirc/0 (defined as LIRC_DRIVER_DEVICE). If /dev/lirc0 is a better default, we should think about patching lirc.

As alternative we could provide two NixOS options for driver and device.

@aakropotkin
Merge branch 'master' of https://github.com/nixos/nixpkgs
ff4952a
@aakropotkin
genymotion: refactor src url
6b273e3 
Fixed revisions:
1) Uses `pname`.
2) URL uses version variable in path. Data type for `url` was changed from path to string.
@aakropotkin
genymotion: refactor src url
b7c6356 
The last commit mistakenly reverted the URL to "old" again. The change has been re-applied.
@aakropotkin
Copy link
Contributor Author

Sorry I did not mean to revive this. This has been sitting without interest for a while. I'm closing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92 Mic92 left review comments

@infinisil infinisil Awaiting requested review from infinisil

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 1-10 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@aakropotkin @markuskowa @ck3d @infinisil @Mic92 @GrahamcOfBorg @Janik-Haag