Provide MAX31865 support #1168
Provide MAX31865 support #1168
Conversation
bijington
left a comment
bijington
left a comment
There was a problem hiding this comment.
This is draft form as I need to actually build this (dev environment isn't happy right now) and also I wanted to check whether this approach works for you. I can then add in the remaining bits like xml docs, etc.
| /// and ISpiPeripheral interface for SPI communication. It follows the IDisposable pattern | ||
| /// for proper resource management. | ||
| /// </remarks> | ||
| public class Max31865 : SamplingSensorBase<Temperature>, ITemperatureSensor, ISpiPeripheral, IDisposable |
There was a problem hiding this comment.
I have added Sampling support into this sensor, is this ok or going too far for a v1?
There was a problem hiding this comment.
Sampling is fine. It largely depends on how the consumer uses it, though my feeling is that sampling is likely outside the purpose of a driver unless the hardware itself provides it. We're going to improve the SensorService to provide sampling at some point, but for now this is fine.
There was a problem hiding this comment.
I'm happy to remove it as I get your point about the driver level
I take it back, I know what lead to this now - I saw that in my local development ITemperatureSensor implements ISamplingSensor which made me think I needed to support sampling. Although I see the Max6675 doesn't do this. I need to dig some more...
There was a problem hiding this comment.
Yeah, ITemperatureSensor shouldn't (And I think in the latest doesn't) implement ISamplingSensor.
There was a problem hiding this comment.
I have moved away from sampling now
| } | ||
| } | ||
|
|
||
| public interface IResistanceTemperatureDetectorConverter |
There was a problem hiding this comment.
This might be a case of over engineering but if you look at the AN709_0.pdf data sheet that I have included there are multiple algorithms for calculating the temperature from the output of the RTD device. My initial thought was to use the Direct Mathematical Method which most other libraries support but leave it open to easy extension if you wanted. I would be happy to remove this concept and just use the conversion directly in the driver if you prefer?
There was a problem hiding this comment.
My gut would be to just have a virtual Convert on the class instead of the interface, so if someone wanted to change conversion math, they could subclass and override it.
| /// wait between readings. This value influences how often `*Updated` | ||
| /// events are raised and `IObservable` consumers are notified. | ||
| ///</param> | ||
| public override void StartUpdating(TimeSpan? updateInterval = null) |
There was a problem hiding this comment.
You could also just derive from SamplingSensorBase.
There was a problem hiding this comment.
I have moved away from sampling now
There was a problem hiding this comment.
For info, these files are auto-generated by the build system, so really you can just check in an empty file and it will magically get generated
|
@ctacke thank you for the review. I have applied the changes that you suggested - I haven't resolved comments as I wanted to check how you like to work with these - do you want to resolve them or should I? Also I should add that I am currently unable to compile this code to prove it compiles and therefore works. I have similar code working in my own codebase but I would like to confirm it is the same code. I need to workout how to fix my local environment as it fails to restore NuGet packages and build under this fork |
|
Sorry I've not had a chance to prove this actually builds yet. I am hoping to find some time this week to test it before marking as ready |
|
Are there any special steps I need in order to build this stuff? I have tried a number of attempts on my Mac but failed miserably to get the Meadow.Foundation solution to build. |
3 participants
This PR provides support for the MAX31865 board and therefore support for the PT100 and PT1000 RTDs.