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

drivers/sensors: Wrote a driver for the MCP9600 thermocouple amplifier. #15525

Merged
merged 1 commit into from
Jan 14, 2025

Conversation

linguini1
Copy link
Contributor

Summary

This PR is from InSpace.

This pull request introduces a new sensor driver for the Mirochip MCP9600 thermocouple amplifier. It is on a popular Adafruit breakout for hobbyists and also happens to be the amplifier used on our hybrid rocket control system.

This PR also comes with a documentation page for how to use the SNIOC commands with this device.

Impact

This PR adds another usable device to the NuttX roster. It will be appreciated by hobbyists that own this board.

Testing

Tested using a Raspberry Pi Pico connected to the Adafruit breakout module. I connected the thermocouple owned by InSpace and verified that the temperature readings were correct.

Tested with K type thermocouple.

@github-actions github-actions bot added Area: Documentation Improvements or additions to documentation Area: Sensors Sensors issues Board: arm Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces. labels Jan 13, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 13, 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.

Yes, this PR appears to mostly meet the NuttX requirements, but could be improved with more specific information.

Here's a breakdown of what's good and what needs improvement:

Strengths:

  • Clear Summary: The summary explains the "what" and "why" of the change, referencing a specific hardware component. Mentioning its use in a real-world application (hybrid rocket control system) adds context.
  • Impact on Users Addressed: The impact on users is acknowledged, highlighting its benefit for hobbyists.
  • Testing Information Provided: Testing details include the hardware platform (Raspberry Pi Pico, Adafruit breakout), the specific thermocouple type (K type), and a confirmation of correct readings.

Weaknesses & Areas for Improvement:

  • Missing Issue References: If there are any related NuttX or NuttX Apps issues, they should be linked. Even if there aren't directly related issues, it might be helpful to link to any relevant discussions or feature requests.
  • Functional Code Change Description Missing: The summary lacks a concise explanation of how the change works. What files were modified? Where in the driver architecture does this new driver fit?
  • Impact Section Needs More Detail: While the user impact is addressed, the other impact categories are either answered with "NO" or lack sufficient detail. For example:
    • Build Impact: Are there any Kconfig options added? Does this driver introduce any new dependencies?
    • Hardware Impact: Specify the architecture (probably arm or armv7-m for the RP2040). Explicitly state if only the RP2040 is supported or if this driver is designed to be portable.
    • Documentation Impact: Stating that a documentation page is included is good, but where is this documentation located? Is it in the NuttX docs directory?
    • Security, Compatibility: Even if the answer is "NO," briefly justify why. For example, "Security Impact: NO. This driver does not introduce any new network interfaces or access sensitive data."
  • Testing Logs Missing: The template specifically requests "Testing logs before change" and "Testing logs after change." Provide actual log output to demonstrate the driver's functionality and the change in behavior. Even simple output showing temperature readings before and after the driver is loaded would be helpful.
  • Build Host Information Missing: The "Testing" section should also specify the build host OS, CPU architecture, and compiler used.

Example of Improved Testing Section:

## Testing

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

* Build Host: macOS Ventura, Apple M1, GCC 12.2.0
* Target: armv7-m (Raspberry Pi Pico), board: raspberrypi-pico

Testing logs before change (without the driver):

nsh> i2c_register /dev/i2c0 0x67 # Register the MCP9600's I2C address
nsh> cat /dev/therm0 # Attempting to read from the thermocouple device
nsh: cat /dev/therm0 failed: -ENODEV (Device not found)


Testing logs after change (with the driver):

nsh> i2c_register /dev/i2c0 0x67
nsh> cat /dev/therm0
25.5 # Example temperature reading in Celsius

By addressing these weaknesses and providing more specific information, the PR will be significantly stronger and easier for reviewers to evaluate.

drivers/sensors/mcp9600.c Outdated Show resolved Hide resolved
drivers/sensors/mcp9600.c Outdated Show resolved Hide resolved
drivers/sensors/mcp9600.c Outdated Show resolved Hide resolved
drivers/sensors/mcp9600.c Outdated Show resolved Hide resolved
include/nuttx/sensors/mcp9600.h Outdated Show resolved Hide resolved
drivers/sensors/mcp9600.c Outdated Show resolved Hide resolved
drivers/sensors/mcp9600.c Outdated Show resolved Hide resolved
drivers/sensors/mcp9600.c Outdated Show resolved Hide resolved
drivers/sensors/mcp9600.c Outdated Show resolved Hide resolved
@raiden00pl
Copy link
Member

Hi @linguini1 , have you considered making this driver compatible with uorb sensor framework which is more portable solution? I know that NuttX doesn't have any official guidelines regarding sensors implementation, but if we use the old method for new sensors, we will never migrate to the new framework...

@linguini1
Copy link
Contributor Author

Hi @linguini1 , have you considered making this driver compatible with uorb sensor framework which is more portable solution? I know that NuttX doesn't have any official guidelines regarding sensors implementation, but if we use the old method for new sensors, we will never migrate to the new framework...

Actually yes, however I must admit I took the path of least resistance on this implementation since the Nuttx documentation about uorb is empty, and the only other information I could find about it was this video. I would be willing to revise this driver to the uorb framework because I agree with your point. It may take me a little bit longer while I explore the code to try to learn more about how it works.

If you happen to know of any doc pages about this that I've missed please send them my way!

@raiden00pl
Copy link
Member

@linguini1 that's the biggest problem with this new framework - there is absolutely no documentation :( and I know that without proper documentation, it's inappropriate to ask contributors to write drivers based on this new framework.
The best source of knowledge at this point is the code (interface in include/nuttx/sensors/sensor.h is nicely described) and all sensors implemented in this way (/drivers/sensors/XXXX_uorb.c). At this moment there is no thermocouple ICs supported this way, so most likely the interface itself needs to be extended.

@linguini1
Copy link
Contributor Author

@linguini1 that's the biggest problem with this new framework - there is absolutely no documentation :( and I know that without proper documentation, it's inappropriate to ask contributors to write drivers based on this new framework.

Just a little elbow grease is all! From what I had seen when I checked it out earlier, it does appear to be a very powerful tool. My rocketry team particularly likes the ability to create fake sensors for mocking our applications, so for us I suppose the extra work would be worth it. Once I get some kind of an understanding I would be happy to contribute a little documentation on the subject if it makes it easier for the next developer.

The best source of knowledge at this point is the code (interface in include/nuttx/sensors/sensor.h is nicely described) and all sensors implemented in this way (/drivers/sensors/XXXX_uorb.c). At this moment there is no thermocouple ICs supported this way, so most likely the interface itself needs to be extended.

Sounds excellent, thank you for the pointers! I will have a look at implementing this!

@xiaoxiang781216
Copy link
Contributor

@Donny9 could you provide some document about urob? let's merge this implementaton first and add uorb later.

@xiaoxiang781216 xiaoxiang781216 merged commit 36507cc into apache:master Jan 14, 2025
40 checks passed
@Donny9
Copy link
Contributor

Donny9 commented Jan 15, 2025

@

@linguini1 that's the biggest problem with this new framework - there is absolutely no documentation :( and I know that without proper documentation, it's inappropriate to ask contributors to write drivers based on this new framework.

Just a little elbow grease is all! From what I had seen when I checked it out earlier, it does appear to be a very powerful tool. My rocketry team particularly likes the ability to create fake sensors for mocking our applications, so for us I suppose the extra work would be worth it. Once I get some kind of an understanding I would be happy to contribute a little documentation on the subject if it makes it easier for the next developer.

The best source of knowledge at this point is the code (interface in include/nuttx/sensors/sensor.h is nicely described) and all sensors implemented in this way (/drivers/sensors/XXXX_uorb.c). At this moment there is no thermocouple ICs supported this way, so most likely the interface itself needs to be extended.

Sounds excellent, thank you for the pointers! I will have a look at implementing this!

@linguini1 hello, you can know everything about uorb and new sensor driver model by doc, PR #15557

@linguini1 linguini1 deleted the mcp9600-driver branch January 17, 2025 03:02
linguini1 added a commit to linguini1/nuttx that referenced this pull request Jan 19, 2025
…r suggestions on PR apache#15525.

The sensor driver now registers three temperature topics and supports the UORB interface.
linguini1 added a commit to linguini1/nuttx that referenced this pull request Jan 20, 2025
…r suggestions on PR apache#15525.

The sensor driver now registers three temperature topics and supports the UORB interface.
linguini1 added a commit to linguini1/nuttx that referenced this pull request Jan 20, 2025
linguini1 added a commit to linguini1/nuttx that referenced this pull request Jan 20, 2025
linguini1 added a commit to linguini1/nuttx that referenced this pull request Jan 20, 2025
xiaoxiang781216 pushed a commit that referenced this pull request Jan 21, 2025
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: Sensors Sensors issues Board: arm Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants