Added modbus agent #638

DaveBoettger · 2024-03-11T18:35:00Z

Modbus agent should work with most Modbus devices simply by supplying the correct configuration file.
Specifically, this has been tested on the following devices on site: -VLT cooling loop pump controllers
-ELNet power monitors
-DSE 8610mkII generator controllers
Sample configuration files for each device are included.

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.

Modbus agent should work with most Modbus devices simply by supplying the correct configuration file. Specifically, this has been tested on the following devices on site: -VLT cooling loop pump controllers -ELNet power monitors -DSE 8610mkII generator controllers Sample configuration files for each device are included.

for more information, see https://pre-commit.ci

BrianJKoopman

Thanks for the PR @DaveBoettger! A few comments and questions below. Biggest thing I'd say is the docs page. Some unit tests would be great, and I'm happy to help get that started, details in the comments.

BrianJKoopman · 2024-03-13T18:57:54Z

socs/agents/modbus/sample_configs/ELNet_General.yaml

I don't think we want to package these into the published package on PyPI. For lack of a better place to put them at the moment, can we move them to /examples/configs/modbus/ at the root of the repository?

BrianJKoopman · 2024-03-13T19:05:08Z

socs/agents/modbus/agent.py

Thanks for being so thorough with the docstrings! In order to easily view them can you create a docs page for this agent? It should live in docs/agents/modbus_agent.rst. There are many examples in that directory, and also documentation here for creating these pages.

To include the agent in the side bar, the new docs .rst page must be added to the docs/index.rst file, alphabetically, in the "Agent Reference" section.

BrianJKoopman · 2024-03-13T19:12:46Z

socs/agents/modbus/agent.py

+    """
+
+    config_dir = os.environ.get('OCS_CONFIG_DIR')
+    path = os.path.join(config_dir, 'modbus_configs', dir_name)


I would leave out 'modbus_configs', let the user decide what directory structure/naming they want. Otherwise this forces $OCS_CONFIG_DIR/modbus_configs/<user defined directory>.

BrianJKoopman · 2024-03-13T19:13:21Z

socs/agents/modbus/agent.py

+
+def load_configs(dir_name, config_extension='yaml'):
+    """
+    Loads all register configuration files form the specified directory (path).


"file form" -> "files from"

BrianJKoopman · 2024-03-13T19:24:39Z

socs/agents/modbus/agent.py

+
+
+def twos(val, num_bytes):
+    """Take an unsigned integer representation of a two's compilment integer and return the correctly signed integer


Please wrap lines to 80 characters. Lots of editors support automating this in some fashion, or have extensions to do so.

BrianJKoopman · 2024-03-13T20:11:34Z

socs/agents/modbus/agent.py

+                        help="Starting action for the agent.")
+    pgroup.add_argument("--configdir", type=str, help="Path to directory containing .yaml config files.")
+    pgroup.add_argument("--sample-interval", type=float, default=10., help="Time between samples in seconds.")
+    pgroup.add_argument("--unit_id", type=float, default=1, help="Modbus unit id for module.")


Convention here is to use hyphens instead of underscores, please make this --unit-id, rather than --unit_id.

BrianJKoopman · 2024-03-13T21:07:32Z

socs/agents/modbus/agent.py

+            {"fields":
+                {'Oil_pressure': {'value': 100.0, 'units': 'Kpa'},
+                 'Coolant_temperature': {'value': 20.0, 'units': 'Degrees C'},
+                 'Oil_temperature': {'value': 20.0, 'units': 'Degrees C'},
+                 'Fuel_level': {'value': 100.0, 'units': '%'},
+                 'Charge_alternator_voltage': {'value': 10.0, 'units': 'V'},
+                 'Engine_Battery_voltage': {'value': 10.0, 'units': 'V'},
+                 'Engine_speed': {'value': 4000, 'units': 'RPM'},
+                 'Generator_frequency': {'value': 1.0, 'units': 'Hz'},
+                 ...
+                 'connection': {'last_attempt': 1680812613.939653, 'connected': True}},
+             "address": 'localhost',
+             "timestamp":1601925677.6914878}


This is great to see! However I noticed that the documented fields here seem to differ from the register names in Generator_Engine_Data.yaml, i.e. 'Engine_oil_pressure' vs 'Oil_pressure'. Is this just a case of one of these two locations being out of date?

This documentation is primarily for the OCS control program writer who needs to reference these keys and might not have access to a running instance of the agent to reverse engineer which keys are available. So these sorts of differences matter here, but I realize it's tedious to keep multiple locations up to date.

There's also a sort of unique issue here in that this agent could be monitoring a different device with different registers, and thus different "fields". I'd like to capture that info here too.

My suggestion would be to include a general structure for the session.data, and describe how it's populated, while pointing to the example configuration files for the details of specific field names and possible value types, etc. Still include things that are always true in the general structure, like the timestamp. Sound good?

BrianJKoopman · 2024-03-14T17:50:18Z

socs/agents/modbus/agent.py

+            if self.error_out_of_range:
+                try:
+                    if val < rconfig['min_val']:
+                        val = self.error_val
+                except KeyError:
+                    pass
+
+                try:
+                    if val > rconfig['max_val']:
+                        val = self.error_val
+                except KeyError:
+                    pass


Two questions here:

How does self.error_val get set? It seems right now to always be None. This would run into issues on line 247 with the float() cast, but maybe I'm missing something.

Would it make sense to distinguish between values below 'min_val' and values above 'max_val'?

BrianJKoopman · 2024-03-14T17:55:32Z

socs/agents/modbus/agent.py

+        try:
+            self.read_start = config['read_start']
+        except KeyError:
+            self.read_start = config['page'] * 256


Where does config['page'] come in? I don't see a 'page' key in any of the example YAML files, or any additions of the key elsewhere in the code.

BrianJKoopman · 2024-03-14T18:23:49Z

socs/agents/modbus/agent.py

This would go above and beyond, but it would be useful to see some examples in the form of some unit tests for the code up to line 156, all the bit register code that doesn't involve actually communicating with a device. There are some docs for how to run the tests here.

The tests would live under socs/tests/agents/. There aren't a lot of examples in there, but you could check out the supersync tests.

Happy to talk more about this too.

David Boettger and others added 2 commits March 11, 2024 17:45

[pre-commit.ci] auto fixes from pre-commit.com hooks

a9c00e3

for more information, see https://pre-commit.ci

BrianJKoopman requested changes Mar 14, 2024

View reviewed changes

This was referenced Mar 23, 2024

Adding agent for the Lightning Detector #640

Open

Adding agent to monitor external fuel tanks level. #639

Open

BrianJKoopman added the new agent New OCS agent needs to be created label Oct 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added modbus agent #638

Added modbus agent #638

DaveBoettger commented Mar 11, 2024 •

edited

Loading

BrianJKoopman left a comment

BrianJKoopman Mar 13, 2024

BrianJKoopman Mar 13, 2024

BrianJKoopman Mar 13, 2024

BrianJKoopman Mar 13, 2024

BrianJKoopman Mar 13, 2024

BrianJKoopman Mar 13, 2024

BrianJKoopman Mar 13, 2024

BrianJKoopman Mar 14, 2024

BrianJKoopman Mar 14, 2024

BrianJKoopman Mar 14, 2024



		def twos(val, num_bytes):
		"""Take an unsigned integer representation of a two's compilment integer and return the correctly signed integer

Added modbus agent #638

Are you sure you want to change the base?

Added modbus agent #638

Conversation

DaveBoettger commented Mar 11, 2024 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

BrianJKoopman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveBoettger commented Mar 11, 2024 •

edited

Loading