8000 Add support of input registers while querying modbus sensor. by LvivEchoes · Pull Request #7082 · home-assistant/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add support of input registers while querying modbus sensor. #7082

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
Merged
merged 2 commits into from
Apr 21, 2017
Merged

Add support of input registers while querying modbus sensor. #7082

merged 2 commits into from
Apr 21, 2017

Conversation

LvivEchoes
Copy link
Contributor
@LvivEchoes LvivEchoes commented Apr 13, 2017 •
edited
Loading

Description:

Add support of modbus input registers query. For now modbus sensor support only holding register, but many devices working can provide data only from input registers. So this PR add support of modbus input registers, and also can process returned value in 32 bit IEEE 754 floating point format.

Added attributes register_type and data_type.

Related issue (if applicable):

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

Example entry for configuration.yaml (if applicable):

  - platform: modbus
    registers:
      - name: Voltage
        unit_of_measurement: V
        slave: 1
        register: 0
        count: 2
        data_type: float
        register_type: input
      - name: Current
        unit_of_measurement: I
        slave: 1
        register: 6
        count: 2
        data_type: float
        register_type: input
        precision: 2

Checklist:

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

@mention-bot
Copy link

@LvivEchoes, thanks for your PR! By analyzing the history of the files in this pull request, we identified @persandstrom, @kixam and @fabaff to be potential reviewers.

@homeassistant
Copy link
Contributor

Hi @LvivEchoes,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@LvivEchoes LvivEchoes mentioned this pull request Apr 13, 2017
Merged
emlove
emlove requested changes Apr 13, 2017
View reviewed changes
Copy link
Contributor
@emlove emlove left a comment •
edited
Loading

Choose a reason for hiding this comment

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

See #7082 (comment)

The input register addition is good, but we shouldn't try to convert anything to a float here. There's no Modbus standard, so the encoding varies by application. We should leave that to be layered on top of the sensor entity.

@LvivEchoes
Copy link
Contributor Author

OK Thank you. What is you advices? add only input register to modbus component, and then develop own sensor type, which can decode value to IEEE-754 standard?

@emlove
Copy link
Contributor
emlove commented Apr 13, 2017 •
edited
Loading

We should get an opinion from @balloob before starting, but these are a few possible approaches:

  1. Perform conversion to float in the modbus sensor platform. Expose the possible variations as configuration options.

  2. Add support to the templates for unpacking these floats. This would allow it to be created as a template sensor, where the user could provide the necessary arguments for byte order, swap endianness, etc.

  3. Create a new sesnor platform that uses another sensor as its input and performs this operation.

  4. Do the same as two, but keep it as a custom component, if we don't think it should be included in hass.

@LvivEchoes
Copy link
Contributor Author

Ok nice. But I would like to add that all Eastron devices http://www.eastrongroup.com/ working in this data type =(

@emlove
Copy link
Contributor
emlove commented Apr 13, 2017

Now that I'm looking at this more, and see the other options, I think I agree that it probably makes the most sense to perform the conversion in this platform. It's unfortunate that there is no standard, since that means we'll need to expose configuration options for the different encoding variations, but it makes the most sense for this sensor to expose the meaningful value for the device. It's not too different from workaround configs in other platforms. Let's wait for @balloob to decide.

@LvivEchoes
Copy link
Contributor Author

Hello @armillis. Can we reach @balloob with another way to decide?

@emlove
Copy link
Contributor
emlove commented Apr 17, 2017

I'll go ahead and make the executive decision that this is OK with the justification posted above. :) It's limited to this platform so it's not too broad.

I think we should restructure the config format a bit though to make this a little more future-proof. Instead of isfloat, let's make the new config option data_type, and define it as follows:

CONF_DATA_TYPE = 'data_type'

DATA_TYPE_INT = 'int'
DATA_TYPE_FLOAT = 'float'

...

    vol.Optional(CONF_DATA_TYPE, default=DATA_TYPE_INT): 
        [DATA_TYPE_INT, DATA_TYPE_FLOAT],

This will give us room to expand when we encounter other wacky Modbus devices.

@balloob
Copy link
Member
balloob commented Apr 20, 2017

Sorry, have been busy. I'm with @armills 👍

@LvivEchoes
Copy link
Contributor Author

Hi @armills. PR updated!

@emlove emlove merged commit bbeb64e into home-assistant:dev Apr 21, 2017
@emlove
Copy link
Contributor
emlove commented Apr 21, 2017

Thanks for the contribution! 🎉

@balloob balloob mentioned this pull request Apr 21, 2017
Merged
@home-assistant home-assistant locked and limited conversation to collaborators Aug 12, 2017
@ghost ghost removed the platform: sensor.modbus label Mar 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@emlove emlove emlove approved these changes

Assignees
No one assigned
Labels
cla-signed integration: modbus
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@LvivEchoes @mention-bot @homeassistant @emlove @balloob
0