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

Asking to enable hardware features that are already enabled #41

Closed
lurch opened this issue Mar 14, 2016 · 12 comments
Closed

Asking to enable hardware features that are already enabled #41

lurch opened this issue Mar 14, 2016 · 12 comments

Comments

@lurch
Copy link
Contributor

lurch commented Mar 14, 2016

I just re-ran the drumhat installer, in order to pull in @Gadgetoid 's recent updates, and during the install it printed:

Checking hardware requirements...

Hardware requires I2C, enable now? [y/N] y

This script will enable I2C on your Raspberry Pi

--- Warning ---

Always be careful when running scripts and commands
copied from the internet. Ensure they are from a
trusted source.

If you want to see what this script does before
running it, you should run:
    \curl -sS get.pimoroni.com/i2c


I2C Already Enabled

I realise it would mean extra complexity would need to be added into all your 'get' scripts, but it would be nice if the drumhat script could detect that I2C was already enabled, and therefore not ask me if I want to enable I2C (which I did above by pressing 'y'). All I'm suggesting is that the "detection" code be added to all the scripts, it still makes sense for the "enablement" code to be kept separately in get.pimoroni.com/i2c.

@RogueM
Copy link
Contributor

RogueM commented Mar 14, 2016

yes, that has bothered me for a while. My preference would be to move the confirmation to the hardware scripts... at least I think it can be achieve that way, I just need to implement the 'warning' in a different way so it only gets displayed if the script does anything to the system.

@RogueM
Copy link
Contributor

RogueM commented Mar 17, 2016

PR #42 should resolve this issue... I brought some of the (most common) detection logic in the template as you suggested, and delegated the opt-out prompts of changing current state to the i2c and spi scripts.

anyhow, only a few scripts moved to this strategy right now while I do more testing. Not live yet, but those that are under the new rules will be shortly.

@RogueM
Copy link
Contributor

RogueM commented Mar 17, 2016

@Gadgetoid and I were discussing the need to probe permission to enable i2c and spi... be it the old system or the new I must admit it makes little sense.

I think the cases where someone would opt-out enabling the required interface but wishes to continue with install are few/inexistent.

I guess, if not suppressed altogether they should probably be lumped together in some fashion with the initial request to confirm the user wishes to go ahead with the main script?

What I'm thinking is that in the initial prompt, the information that some interfaces need to be enabled as part of the process should be stated - they are likely the most relevant reason to abort in fact (although more likely for UART vs console).

Thoughts?

@RogueM
Copy link
Contributor

RogueM commented Mar 18, 2016

PR #43 includes a different implementation of the interaction the user has to provide in respect to hardware feature, namely:

interface enabling is no longer prompted - instead the requirement of the hardware is provided upfront and user can decide at that point to abort, if they don't interface enabling will be done automatically.

I have maintained the detection in the main script for I2C and SPI from PR #42, though it is no longer strictly required, but it makes the scripts more self-contained and pulling the delegate won't occur if there is nothing to be done.

@lurch
Copy link
Contributor Author

lurch commented Mar 18, 2016

IMHO the average user is unlikely to know whether their i2c/spi/uart ports are enabled or disabled, so it might be nice if the scripts said something like "Note: $productname requires I2C communication, which is already enabled" or "Note: $productname requires I2C communication, which this script will need to enable" ?
And that would mean you wouldn't need to show the warning "The serial console will be disabled if you proceed!" if the uart is already enabled.

P.S. WRT enabling the UART, are you aware of the issues surrounding the UART on the Pi3? RPi-Distro/repo#22 and raspberrypi/firmware#553
(I dunno which, if any, of the Pimoroni products use the UART)

@RogueM
Copy link
Contributor

RogueM commented Mar 18, 2016

the intend of those warnings is not to reflect the state of I2C or SPI (UART is not even probed), just infom user that this is a requirement.

and, yup serial of Pi3 is quite a mess, I'm following the development closely. Fortunately only iOT pHAT currently requires UART so primarily not aimed at Pi3 (although there is nothing preventing users).

@lurch
Copy link
Contributor Author

lurch commented Mar 18, 2016

I just think that the fewer warnings you show to the user, the less 'scared' they'll be. And detecting whether required hardware features are already enabled or not is one way to cut down on the unnecessary warnings.

But it's obviously your call...

@RogueM
Copy link
Contributor

RogueM commented Mar 18, 2016

fair point... on the other hand if people don't realise console and UART are mutually exclusive they may be in for a rude awakening...

@Gadgetoid
Copy link
Member

i2c and spi are easy enough to detect, too, since you can check for the presence of /dev/i2c* and /dev/spi* without doing any nasty config parsing gymnastics.

@RogueM
Copy link
Contributor

RogueM commented Mar 18, 2016

yes, exactly... I looked into it and decided it wasn't worth it for UART, at this point anyhow. And that was before the Pi3 was released ;-)

Ultimately I don't feel strongly about the I2C and SPI 'notes', but I don't think they are intimidating. I think UART should be stressed adequately however, it's a service to the user IMO but I'm happy to downgrade it to same level i.e informational.

@RogueM
Copy link
Contributor

RogueM commented Mar 18, 2016

but to clarify again, those 'notes' are not there to pre-emptively inform the user of what will happen when the script is ran, the detection itself is performed later, by design.

With the removal of the prompt I just feel that users need to register at least at initial install what interface are required, so that they hopefully don't mess with raspi-config later on and disable it again :/

@RogueM
Copy link
Contributor

RogueM commented Mar 18, 2016

PR #44 tones down some of the inline warnings. Informational notes remain unchanged compared to PR #43

@RogueM RogueM closed this as completed Mar 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants