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

Made it possible to define multiple Octoprint printers #16519

Merged
merged 17 commits into from Oct 11, 2018

Conversation

Projects
None yet
5 participants
@reefab
Contributor

reefab commented Sep 9, 2018

Description:

The current Octoprint platform supports only one device. I've removed this limitation by making it possible to define multiple printers during configuration.

Breaking change: the configuration of the Octoprint platform needs to be converted to a list.

Successor of #16508

Related post: https://community.home-assistant.io/t/octoprint-support-for-multiple-printers/29959

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#6239

Example entry for configuration.yaml:

Previous syntax:

octoprint:
  host: 
  api_key: 

sensor:
  - platform: octoprint
    monitored_conditions:
      - Current State

New syntax:

octoprint:
  - host: 
    api_key: 
    name: printer1
    sensors:
      monitored_conditions:
        - Current State
  - host: 
    api_key: 
    name: printer2

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If user exposed functionality or configuration variables are added/changed:

@reefab

This comment has been minimized.

Contributor

reefab commented Sep 9, 2018

screen shot 2018-09-09 at 16 43 59

Screenshot with 2 printers and the sensor and binary_sensor enabled for both.

@w1ll1am23

This comment has been minimized.

Collaborator

w1ll1am23 commented Sep 9, 2018

👍 Looks good to me.

@reefab

This comment has been minimized.

Contributor

reefab commented Sep 10, 2018

I added a configuration option for octoprint's port.

The default for this component was, and still is, 80 but Octoprint's default is actually 5000.

As I'm guessing 99% of Octoprint users are actually using Octopie, a pre-installed raspberry pi image, that has a reverse proxy redirecting Octoprint's port to 80, it probably wasn't an issue but I wanted to make sure the Octoprint component could actually connect to a vanilla Octoprint installation if needed.

@reefab reefab closed this Sep 12, 2018

@reefab reefab reopened this Sep 12, 2018

@wafflebot wafflebot bot added in progress and removed in progress labels Sep 12, 2018

Show resolved Hide resolved homeassistant/components/octoprint.py Outdated
Show resolved Hide resolved homeassistant/components/binary_sensor/octoprint.py Outdated
Show resolved Hide resolved homeassistant/components/octoprint.py
Show resolved Hide resolved homeassistant/components/octoprint.py Outdated
@reefab

This comment has been minimized.

Contributor

reefab commented Oct 6, 2018

By the way, I noticed that @w1ll1am23 added support in netdisco for auto-discovering Octoprint servers.

This probably will be better in a subsequent PR, but we can combine this along the auto platform loading to have an integration where the servers will be automatically detected and the user will just need to add the API key in the UI to get a working monitoring of their printer.

I'm reading the documentation for the config flow handlers but as I said it should probably be in a second PR.

@MartinHjelmare

Should we fail component setup if no printers were able to be set up?

@reefab

This comment has been minimized.

Contributor

reefab commented Oct 9, 2018

I'm not quite sure of the implications of failing component setup in that case. What would you recommend?

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Oct 10, 2018

It's more clear to the user if we fail component setup that there's a problem with the setup. Since no platforms will have loaded, no entities will be created and never will, so I think we should fail component setup in that case. The user will have to fix the problem and restart home assistant to trigger another setup.

@reefab

This comment has been minimized.

Contributor

reefab commented Oct 10, 2018

Done, I return False if the platform had no printer added.

"""Validate that printers have an unique name."""
names = []
for printer in value:
name = printer['name'] if 'name' in printer else DEFAULT_NAME

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 10, 2018

Member
name = printer.get('name', DEFAULT_NAME)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 10, 2018

Member

Even better, move the call to has_all_unique_names in the config schema after the main part in the schema, ie after [vol.Schema({...}]. Then default values will have been populated in the dict.

This comment has been minimized.

@reefab

reefab Oct 10, 2018

Contributor

I just tried that but unfortunately it didn't gave me a very usable result: If I move the call to has_all_uniques_names to the next vol.Schema.

Going from:

  DOMAIN: vol.All(cv.ensure_list, has_all_unique_names, [vol.Schema({

to (if I understood correctly):

  DOMAIN: vol.All(cv.ensure_list, [vol.Schema(has_all_unique_names, {

has_all_uniques_names gets called once for each entry, instead of being called only once with all entries. It makes verifying all the names at once for uniqueness more difficult.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 10, 2018

Member

No it should be:

DOMAIN: vol.All(cv.ensure_list, [vol.Schema({...})], has_all_unique_names),

I might have forgotten a parenthesis in my earlier comment.

This comment has been minimized.

@reefab

reefab Oct 10, 2018

Contributor

Got it, that makes more sense. I changed it that way and it indeed simplified the custom validator.

continue
sensors = printer[CONF_SENSORS][CONF_MONITORED_CONDITIONS]
load_platform(hass, 'sensor', DOMAIN, {'name': name,

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 10, 2018

Member

An optimization would be to merge the discovery_info for each printer into one container and call load_platform only once per domain. But it's not strictly necessary.

@MartinHjelmare

Nice!

@MartinHjelmare MartinHjelmare merged commit 9fa7906 into home-assistant:dev Oct 11, 2018

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.08%) to 93.48%
Details

@wafflebot wafflebot bot removed the in progress label Oct 11, 2018

@balloob balloob referenced this pull request Oct 26, 2018

Merged

0.81 #17809

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