Simplify MCP3008 #663
Simplify MCP3008 #663
Conversation
|
ping @joperezr |
| int configurationBits = (0b0001_1000 | channel) << 3; | ||
| // first we send 5 bits, rest is ignored | ||
| // start bit = 1 | ||
| // conversion bit = 1 (single ended) |
There was a problem hiding this comment.
Why did we stop allowing the Differential configuration?
There was a problem hiding this comment.
it's not useful. Previously the support was very limited - it wasn't clear what it was subtracting exactly or how to use it.
The software implementation for the differential configuration between channel 0 and 1 is just:
adc.Read(1) - adc.Read(0) so there is no clear usage for that as subtraction is just easier to use.
I'd recommend not having it until there is an actual scenario where you can achieve something you cannot with subtracting (which I doubt there is such)
There was a problem hiding this comment.
I'd recommend not having it until there is an actual scenario where you can achieve something you cannot with subtracting (which I doubt there is such)
I think this is why there is a need for better guidance/standards on expectations for binding development. When I work on bindings (especially types with registers or memory locations that can be modified), I look at trying to develop the API based on the manufacture datasheet. This includes values/properties (usually named based on how the datasheet references/labels them) to allow an easy way to 1) understand/search between the API and datasheet and 2) pass in parameters that will change bits/bytes to control the device down to the lowest level. There's been previous discussions on not including enums or properties based on registers. I disagree as it offers clarity and probably easier for the majority of others to get started faster. But I also agree it can make the API larger and sometimes overwhelming to beginners that don't have experience with circuits and datasheets. That's why they start with the Samples page.
Of course, the binding API should be convenient and help configure the device for most use-case scenario. However, the API shouldn't limit functionality as others might need that granularity and now can't use the API for their needs. In the case of this binding (Mcp3008), @richlander original added a very limited API. This was understandable, as we were trying to get our minds wrapped around how to build upon the core API and show some samples. I later updated with fixes including the InputConfiguration (as based on the datasheet) to help select type of channel. I realize this binding was going to change as we would eventually create new ADC APIs and implement like it was done with Mcp23xxx and GpioController.
There are cases where one might need a couple single and a couple diff channels. There are cases where hardware designers use them to measure signals from on-board components and connectors (from external wire harnesses). Functionality is reduced by removing this parameter. Yes, the substraction scheme may work in case (I've not taken time to review in detail). You also lose time as you would have to perform 2 channel reads and calculate where diff channel reads take 1.
Why not just change the parameter to optional so user has the option as it might be a use-case for thier needs? Afterall, the device offers it and the datasheet helps explain.
BTW, I'm not trying to 💩 on the improvements made in this PR. Just a little frustrated we may continue to go in circles trying to update what surface area is expected for bindings. The guidance doc would be a big help here. That's one reason I've held off on working on current assigned bindings.
For example...
- Add CY8C95xx I/O Expander with EEPROM Binding #225: PwmController was removed and not a good way to manage related channels now.
- Pca95x4 should implement GpioDriver #226: Looks like there's talks (from Project Board) on removing IGpioController.
There was a problem hiding this comment.
Current state on guidelines for device bindings is following:
Only the most useful APIs should be public, anything else which may be useful but will unlikely get used by most of the people should be protected (inheriting the class allows you to use it but it is not visible by default) or internal/private
I do understand the point of 1:1 mapping the spec but at the same time specs are not easily the easiest source of reference (especially regarding how stuff should be used).
For some devices we may accept 1:1 mapping but there should also exist a simple to use version in such case
There was a problem hiding this comment.
I've opened #673 please let me know what do you think is missing there or we should clarify
| @@ -29,41 +23,36 @@ public void Dispose() | |||
| _spiDevice = null; | |||
| } | |||
|
|
|||
| public int Read(int channel, InputConfiguration inputConfiguration = InputConfiguration.SingleEnded) | |||
| public int Read(int channel) | |||
There was a problem hiding this comment.
Given that you are already here and that we want to support this binding officially, can you add xml docs to all of the three public methods and to the class?
Mainly remove InputConfiguration but also:
Fixes TODO on one of our 3.0 items: https://github.com/dotnet/iot/projects/4#card-24042553