Conversation
|
@m-kormann Thanks for offering this PR! I have no experience with IIO and I will need to get my head around it first. Next days are pretty filled up, so please bear with me if it takes some time. Seems IIO does similar things like Comedi. From the example, I understand that IIO can be used out-of-the-box on Pinephone and Raspberry Pi. Maybe you could drop a few lines about your motivation for using IIO and how it is positioned to Comedi? |
|
You are very welcome! This example emerged from personal interest when I found that the data of the phone's gyroscope can be sent over network. This opens a lot of applications to real-time models. From my understanding, Comedi is an interface to measurement with the help of certain hardware (even Soundcards) to measure voltages. It is restricted to the list of drivers available for the certain measurement hardware. IIO is a layer above the I2C-Bus, which communicates with I/O hardware. As many arduino expansion boards (i.e. gyroscopes, magnetometers, proximity sensors) use I2C, a lot of devices, drivers and usage examples are available in the communities. |
There was a problem hiding this comment.
I only checked the header file so far and wondered about the usage of the static storage class which should be avoided. There are also some minor style and cosmetic issues.
@bernhard-thiele Did you find some time for evaluation?
tbeu
left a comment
tbeu
left a comment
There was a problem hiding this comment.
Some more statics to be avoided.
Bump. |
No, I only had a superficial view on it. I think it would be cool, but since there was no pressing need to get it in and I have no experience with it, I procrastinated it. My first impression (now some time ago) was that there would be some work needed before it is "feature complete and ready". Unfortunately, at the moment there is not really time for me to work on it, so I'm not sure how to handle it. |
WalkthroughThis pull request introduces several new Modelica models and components to integrate Industrial I/O (IIO) support. A new test model for hardware using InternetIO on Pinephone is added along with supporting blocks and configuration records for accessing IMU data. Additionally, new packages, classes, and functions are provided to wrap functionalities from the libiio library, including opening devices and channels, reading data, and managing device contexts. Package order files and header interfaces (both for libiio and custom MDDIIO functionality) have been updated to include these additions. Changes
Sequence Diagram(s)sequenceDiagram
participant TH as TestHardwareIIO
participant PD as PhysicalDataRead
participant IC as IIOchannel
participant IIO as IIO
participant Lib as MDDIIO/libiio
TH->>PD: Initialize IMU data reading
PD->>IC: Request to open channel
IC->>IIO: Call "Open channel" (constructor)
IIO->>Lib: Execute MDD_iio_open_channel
Lib-->>IIO: Return channel handle
IIO-->>IC: Pass channel handle
IC-->>PD: Confirm channel is open
PD->>Lib: Invoke data_read function
Lib-->>PD: Return sensor data
PD-->>TH: Supply acceleration data
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
Modelica_DeviceDrivers/HardwareIO/IIO_/data_read.mo (2)
2-4: Clarify function documentation for its purpose and return value
The high-level intent is clear ("Read from analog channel"), but the function could use additional contextual insight in its docstring about valid usage scenarios, potential edge cases, and an example of typical attribute reads (e.g., "scale", "raw", etc.).
7-8: Rename 'attrname' for better clarity
The parameter nameattrnameis documented as "Device name", which can be confusing. If it is specifically meant for channel attributes (e.g., "raw", "scale"), consider changing it toattributeNameor clarifying its intended usage.Modelica_DeviceDrivers/Blocks/HardwareIO.mo (3)
329-339: Ensure the documentation matches IIO usage rather than Comedi
In several places, the text references “comedi device,” e.g., in the annotation at line 338. Please confirm or update the documentation so that it accurately reflects this new IIO context and clarifies thatdeviceNameis for an IIO device, not a Comedi device.
363-363: Rename or correct the parameter documentation
Currently documented as “Handle to comedi device,” but the parameter is actually holding an IIO context. Update the parameter description to say “Handle to IIO device” or similar.
398-399: Remove confusing reference to "Comedi function"
Line 399 mentions "Comedi function iio_channel_attr_read_double(..)", which appears to be a leftover doc mismatch. Change it to something like “Usesiio_channel_attr_read_double(...)from libiio” to avoid confusion.Modelica_DeviceDrivers/HardwareIO/IIOchannel.mo (1)
2-4: Consider adding usage details and platform constraints
The class docstring is concise. It may be helpful to emphasize any platform constraints (Linux vs. Windows) and mention typical usage scenarios for users unfamiliar with IIO channels.Modelica_DeviceDrivers/Blocks/Examples/TestHardwareIIO.mo (3)
31-33: Improve initialization parameters for BullsEyeLevel.
Explicitly settingfixed=trueandstart=0forthetaandpsiis often fine, but consider adjusting the initial angles or making them user-configurable in the model’s parameter dialog. That way, the user can easily shift the ball’s initial position if desired.
72-73: Clarify the 2D projection signs.
The negative sign inx2D[1] = -80*sin(theta)*sin(psi);might be intentional, but a short inline comment could help future contributors understand the orientation or reference frame for this top view.
97-100: Expand the documentation with step-by-step setup instructions.
The documentation references installingiiodand mentions it works out of the box, but adding short instructions (e.g., how to configuremobian.local, start the daemon, or definempu6050channels) will help users new to IIO set up the environment more easily.Modelica_DeviceDrivers/HardwareIO/IIO.mo (1)
6-14: Ensure robustness in the constructor.
Currently, the constructor depends on the internal macro-based error handling in C (viaMDD_iio_open). While typical in Modelica, consider handling or propagating an error state back to the user in Modelica if the external function fails (e.g., iflibiiois unavailable).Modelica_DeviceDrivers/Resources/Include/MDDIIO.h (1)
23-34: Avoid overshadowing error messages with a single macro.
The usage ofIIO_ENSUREfor multiple assignments (e.g.,ctx = iio_create_network_context(...)) can be slightly less readable for external collaborators. A more explicit approach could improve maintainability:
- First check that the function call is successful.
- Then log or raise an error if needed.
This refinement helps keep context and error-handling logic clear.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
Modelica_DeviceDrivers/Blocks/Examples/TestHardwareIIO.mo(1 hunks)Modelica_DeviceDrivers/Blocks/Examples/package.order(1 hunks)Modelica_DeviceDrivers/Blocks/HardwareIO.mo(1 hunks)Modelica_DeviceDrivers/HardwareIO/IIO.mo(1 hunks)Modelica_DeviceDrivers/HardwareIO/IIO_/data_read.mo(1 hunks)Modelica_DeviceDrivers/HardwareIO/IIO_/package.mo(1 hunks)Modelica_DeviceDrivers/HardwareIO/IIO_/package.order(1 hunks)Modelica_DeviceDrivers/HardwareIO/IIOchannel.mo(1 hunks)Modelica_DeviceDrivers/HardwareIO/package.order(1 hunks)Modelica_DeviceDrivers/Resources/Include/MDDIIO.h(1 hunks)Modelica_DeviceDrivers/Resources/thirdParty/libiio/iio.h(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
Modelica_DeviceDrivers/Resources/Include/MDDIIO.h (1)
Modelica_DeviceDrivers/Resources/thirdParty/libiio/iio.h (7)
iio_create_network_context(437-437)iio_create_local_context(408-408)iio_context_get_devices_count(578-579)iio_context_destroy(500-500)iio_context_find_device(598-599)iio_device_find_channel(702-703)iio_channel_attr_read_double(1252-1253)
🔇 Additional comments (9)
Modelica_DeviceDrivers/HardwareIO/package.order (1)
3-5: IIO-related entries added correctly to package orderThe entries for
IIO,IIOchannel, andIIO_have been properly added to the package order file, which is necessary for their inclusion in the Modelica package structure. This aligns with the PR objective of adding Industrial I/O (IIO) protocol support.Modelica_DeviceDrivers/HardwareIO/IIO_/package.order (1)
1-1: Entry for data_read function properly addedThe
data_readfunction is correctly listed in the package order file, which is necessary to expose the function in the IIO_ package. This is consistent with the implementation of the data reading capability mentioned in the PR objectives.Modelica_DeviceDrivers/Blocks/Examples/package.order (1)
19-19: Test case added to package orderThe
TestHardwareIIOentry has been appropriately added to the Examples package order file. It's correctly positioned after the similarTestHardwareIOComedientry, maintaining a logical organization in the file.Modelica_DeviceDrivers/HardwareIO/IIO_/package.mo (1)
1-5:Details
✅ Verification successful
IIO_ package structure is well-defined
The package declaration is appropriately structured with proper namespace (
within Modelica_DeviceDrivers.HardwareIO) and includes a descriptive documentation string. The extension ofDriverIconmaintains consistency with other driver implementations in the library.However, the package appears to be empty at this point, containing only the package declaration but no actual implementation or components. The package.order file suggests a
data_readfunction should be included, but it's not present in this file.
🏁 Script executed:
#!/bin/bash # Check if the data_read function is implemented in a separate file echo "Checking for the data_read.mo file in the IIO_ directory:" find Modelica_DeviceDrivers/HardwareIO/IIO_ -name "data_read.mo" -type fLength of output: 249
IIO_ package structure verification complete
- The package declaration in
Modelica_DeviceDrivers/HardwareIO/IIO_/package.mois correct, with appropriate namespacing and extension ofDriverIcon.- Although the package file itself is minimal, the expected
data_readfunction is implemented in the separate filedata_read.mo(located inModelica_DeviceDrivers/HardwareIO/IIO_), aligning with the design indicated by thepackage.orderfile.Modelica_DeviceDrivers/HardwareIO/IIO_/data_read.mo (1)
10-10: Verify error handling of the external function call
There is no handling for potential errors or invalid return values fromMDD_iio_data_read. Consider verifying that unexpected conditions (e.g., non-existent attribute) are handled gracefully in upstream or downstream code.Modelica_DeviceDrivers/Blocks/Examples/TestHardwareIIO.mo (2)
6-7: Consider validating the connection in Modelica code.
Currently, theiio(deviceName="mobian.local")block does not handle connection failures or missing devices if the device name is incorrect or unreachable. Making the model robust against these issues (e.g., by exposing an error output or providing a fallback) will improve the user experience.Would you like me to generate a script to search for usage of this device name to see if any fallback is implemented in other parts of the code?
67-68: Double-check division by zero logic.
You already usemax(Modelica.Constants.eps, d/2 * sin(theta))to avoid dividing by zero. Ensure thatdorthetacan’t be externally modified to zero in a way that produces undesired numeric instabilities.Modelica_DeviceDrivers/HardwareIO/IIO.mo (1)
16-23: Validate destructor usage.
Destroying an already closed or invalidIIOcontext might lead to undefined behavior if the call chain from Modelica triggers it multiple times. If feasible in Modelica, confirm that the destructor is only called once or handle repeated destructor calls gracefully in C.Modelica_DeviceDrivers/Resources/thirdParty/libiio/iio.h (1)
1-1975: Confirm third-party licensing and references.
This file is marked with LGPL-2.1-or-later and references Analog Devices, Inc. Typically, third-party headers should remain unmodified to maintain license compliance. Ensure that you have included any required license text, disclaimers, or references in your repository according to that license’s terms.Do you want me to generate a script to scan your repository for license headers and confirm all references to “LGPL-2.1-or-later” are present where required?
| rawData = Modelica_DeviceDrivers.HardwareIO.IIO_.data_read(channel, "raw"); | ||
| y = scaleData*rawData; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential errors or invalid data when reading attributes
Reading "raw" or "scale" could fail or yield invalid data if the attribute is unavailable. Consider adding checks or fallback logic to handle such cases.
| encapsulated function destructor "Close channel" | ||
| import Modelica; | ||
| extends Modelica.Icons.Function; | ||
| import Modelica_DeviceDrivers.HardwareIO.IIOchannel; | ||
| input IIOchannel channel "Channel context"; | ||
| end destructor; |
There was a problem hiding this comment.
Implement actual channel closure logic
The destructor is empty and doesn’t invoke a corresponding external function to close or free resources. This could lead to resource leaks.
Below is a possible fix for calling a matching external function (assuming one exists, e.g., MDD_iio_close_channel):
encapsulated function destructor "Close channel"
import Modelica;
extends Modelica.Icons.Function;
import Modelica_DeviceDrivers.HardwareIO.IIOchannel;
input IIOchannel channel "Channel context";
-end destructor;
+external "C" MDD_iio_close_channel(channel)
+ annotation (Include="#include \"MDDIIO.h\"", Library="libiio");
+end destructor;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| encapsulated function destructor "Close channel" | |
| import Modelica; | |
| extends Modelica.Icons.Function; | |
| import Modelica_DeviceDrivers.HardwareIO.IIOchannel; | |
| input IIOchannel channel "Channel context"; | |
| end destructor; | |
| encapsulated function destructor "Close channel" | |
| import Modelica; | |
| extends Modelica.Icons.Function; | |
| import Modelica_DeviceDrivers.HardwareIO.IIOchannel; | |
| input IIOchannel channel "Channel context"; | |
| external "C" MDD_iio_close_channel(channel) | |
| annotation (Include="#include \"MDDIIO.h\"", Library="libiio"); | |
| end destructor; |
| else { | ||
| IIO_ENSURE(ctx = iio_create_local_context(), "Could not open IIO host", "locally"); | ||
| } | ||
| IIO_ENSURE(iio_context_get_devices_count(ctx) > 3, "No devices found at host", targetname); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Revisit the device-count assumption.
iio_context_get_devices_count(ctx) > 3 might be too strict for some boards or minimal setups. If a device has fewer than four channels, this check will erroneously fail. Consider making this condition configurable or removing it unless you’re certain every supported IIO device must have at least four recognized channels.
- IIO_ENSURE(iio_context_get_devices_count(ctx) > 3, "No devices found at host", targetname);
+ IIO_ENSURE(iio_context_get_devices_count(ctx) > 0, "No devices found at host", targetname);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| IIO_ENSURE(iio_context_get_devices_count(ctx) > 3, "No devices found at host", targetname); | |
| IIO_ENSURE(iio_context_get_devices_count(ctx) > 0, "No devices found at host", targetname); |
3 participants
Industrial I/O (https://wiki.analog.com/software/linux/docs/iio/iio) is a protocol to communicate with integrated devices.
This implementation allows to read sensor data from IIO devices locally or over network.
Summary by CodeRabbit