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

MCP9600 UORB implementation #15605

Merged
merged 1 commit into from
Jan 21, 2025
Merged

Conversation

linguini1
Copy link
Contributor

@linguini1 linguini1 commented Jan 19, 2025

Summary

This PR is from InSpace.

This PR moves the MCP9600 sensor implementation over to the UORB framework instead of the legacy structure, as per suggestions on #15525.

This also adds a dedicated SNIOC command for configuring the thermocouple type of a device (for amplifiers which support multiple types).

I have also updated the documentation of the MCP9600 to reflect the changes.

Finally, this also changes the SENSOR_TYPE_AMBIENT_TEMPERATURE macro to SENSOR_TYPE_TEMPERATURE macro, since the distinction is not required and prevents confusion about some sensor topics being for ambient temperature while others measure the direct temperature of an object.

I have deliberately split the PR into two commits since renaming the macro I believe deserves its own distinct commit with a different message.

Impact

This improves the MCP9600 because it is now compatible with the more powerful UORB framework. This extension also paves the way for other thermocouple amplifiers and object temperature measuring sensors to be integrated into NuttX.

The changing of the macro name was discussed in the developer forums and suggested as an alternative to creating a new type which would be functionally the same as the temperature type, just by another macro name. I have modified all instances of the macro name in the kernel. This should not affect the user code since there are also no applications in the NuttX apps repository that mention the macro by name, but I may have overlooked some other impact.

Testing

Testing was performed similarly to the MCP9600 legacy driver but this time by reading measurements with the uorb_listener application. Rates were configurable and measurements were displayed properly.

nsh> uorb_listener

Mointor objects num:3
object_name:sensor_temp, object_instance:3
object_name:sensor_temp, object_instance:2
object_name:sensor_temp, object_instance:1
sensor_temp(now:12310000):timestamp:12310000,temperature:-0.062500
sensor_temp(now:12310000):timestamp:12310000,temperature:24.437500
sensor_temp(now:12310000):timestamp:12310000,temperature:24.37500
sensor_temp(now:13320000):timestamp:13320000,temperature:-0.062500
sensor_temp(now:13320000):timestamp:13320000,temperature:24.437500

I have also built the documentation locally and verified that it renders correctly in the browser.

@github-actions github-actions bot added Area: Documentation Improvements or additions to documentation Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: xtensa Issues related to the Xtensa architecture Area: OS Components OS Components issues Area: Sensors Sensors issues Board: arm Size: L The size of the change in this PR is large labels Jan 19, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 19, 2025

[Experimental Bot, please feedback here]

Fill In The Commit Message: This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary.

The PR description is a good start but needs improvement to fully meet the NuttX requirements. Here's a breakdown of what's missing and how to improve it:

Summary:

  • More detail on how the UORB integration works: Mention specific UORB topics used for publishing sensor data. For example, "The driver now publishes temperature data to the sensor_temperature topic...". This helps reviewers understand the integration at a glance.
  • Clarify the SNIOC command: Specify the exact command name and the parameters it takes (e.g., "A new SNIOC command, SNIOC_SET_THERMOCOUPLE_TYPE, is added to configure the thermocouple type. This command takes an integer argument representing the thermocouple type...").
  • Justify the macro change more thoroughly: Briefly explain why the distinction between ambient and direct temperature was unnecessary in the first place.

Impact:

  • Impact on user (YES): Users will need to adapt if they were previously using the legacy driver interface or relying on the SENSOR_TYPE_AMBIENT_TEMPERATURE macro. Explain the migration path or any compatibility headers/macros provided. Example: "Users of the legacy MCP9600 driver will need to switch to the UORB interface. Applications relying on SENSOR_TYPE_AMBIENT_TEMPERATURE should now use SENSOR_TYPE_TEMPERATURE."
  • Impact on build (Possibly YES): If any Kconfig options were added or modified, mention them here.
  • Impact on hardware (Possibly YES): Specify which architectures and boards are supported by this driver.
  • Impact on documentation (YES): Explicitly state that documentation updates are included in the PR. Describe what documentation was added or changed (e.g., "Updated the MCP9600 driver documentation to reflect the UORB integration and the new SNIOC command. Added a section on using the sensor_temperature topic.").

Testing:

  • Insufficient detail on build host and target: Provide specific information. Examples:
    • Build Host: Linux (Ubuntu 22.04), x86_64, GCC 11.2.0
    • Target: sim:nsh
    • Target: stm32f4discovery:default
  • Testing logs inadequate: The provided logs only show after the change. Include logs from before the change using the legacy driver to demonstrate the difference and prove that the new driver produces equivalent (or improved) results. Also, show the command used to configure the thermocouple type with the new SNIOC command.
  • More comprehensive testing: Test different thermocouple types if supported. Test different sampling rates. Test error conditions (e.g., what happens if the sensor is disconnected?).

Example of Improved Testing Section:

Testing

I confirm that changes are verified on my local setup and work as intended:

  • Build Host: Linux (Ubuntu 22.04), x86_64, GCC 11.2.0
  • Target 1: sim:nsh
  • Target 2: stm32f4discovery:default (connected to an MCP9600 with a K-type thermocouple)

Testing logs before change (using legacy driver):

nsh> mcp9600_test # or whatever command was used previously
... (output showing temperature readings) ...

Testing logs after change (using UORB):

nsh> mcp9600_ioctl /dev/temp0 SNIOC_SET_THERMOCOUPLE_TYPE 1  # Example: setting K-type
nsh> uorb_listener sensor_temperature
... (output showing temperature readings from the sensor_temperature topic) ...

Further testing included setting different thermocouple types (types 2 and 3) and verifying the temperature readings against a reference thermometer. Different sampling rates were also tested (1Hz, 10Hz, and 50Hz) to confirm correct operation. A test with the sensor disconnected was performed, resulting in expected error messages being logged.

By addressing these points, you'll significantly improve the clarity and completeness of your PR, making it easier for reviewers to understand and approve your changes. Remember to also follow the NuttX coding style and commit message guidelines.

include/nuttx/sensors/ioctl.h Outdated Show resolved Hide resolved
drivers/sensors/mcp9600_uorb.c Outdated Show resolved Hide resolved
@Donny9
Copy link
Contributor

Donny9 commented Jan 20, 2025

@linguini1 let's rebase on #15610, using new type: SENSOR_TEMPERATURE

@github-actions github-actions bot removed Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: xtensa Issues related to the Xtensa architecture labels Jan 20, 2025
@linguini1 linguini1 force-pushed the mcp9600-uorb branch 2 times, most recently from beb2e4f to b342566 Compare January 20, 2025 17:25
@linguini1 linguini1 marked this pull request as draft January 20, 2025 18:18
@linguini1 linguini1 marked this pull request as ready for review January 20, 2025 19:52
@xiaoxiang781216 xiaoxiang781216 merged commit 1d205e9 into apache:master Jan 21, 2025
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Documentation Improvements or additions to documentation Area: OS Components OS Components issues Area: Sensors Sensors issues Board: arm Size: L The size of the change in this PR is large
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants