dotnet / iot

This repo includes .NET Core implementations for various IoT boards, chips, displays and PCBs.
MIT License
2.18k stars 585 forks source link

Device submission guidance #350

Closed PRIMETSS closed 5 years ago

PRIMETSS commented 5 years ago

After starting work on a possible improvement to a device, I thought to check that what I was doing followed the Template for submission on Device. It deviated a bit, and had me go spot check more devices for an understanding of how things like setting Registers are being handled.

Typically Devices (Chips) are commonly a bunch of control registers, which are bit toggled through serial reads and writes through some interface (e.g. I2C). Commonly this is done through bit bashing of Registers & Register Parameter enums through bit masking. e.g. BNO0555.h

These Registers are generally set 'on mass' by writing to the register after OR'ing up all the parameters you wish to set

// Initalise ADS1115 default hardware config as per datasheet, Default Config Register = 0x8583 _config = (ConfigParams.ADS1015_REG_CONFIG_OS_NOTBUSY | ConfigParams.ADS1015_REG_CONFIG_MUX_DIFF_0_1 | ConfigParams.ADS1015_REG_CONFIG_PGA_2_048V | ConfigParams.ADS1015_REG_CONFIG_MODE_SINGLE | ConfigParams.ADS1015_REG_CONFIG_DR_8SPS | ConfigParams.ADS1015_REG_CONFIG_CMODE_TRAD | ConfigParams.ADS1015_REG_CONFIG_CPOL_ACTVLOW | ConfigParams.ADS1015_REG_CONFIG_CLAT_NONLAT | ConfigParams.ADS1015_REG_CONFIG_CQUE_NONE);

Setting or Checking a single Register parameter could be done using a Bit Mask if ((this._config & ConfigParams.ADS1015_REG_CONFIG_MODE_SINGLE) == ConfigParams.ADS1015_REG_CONFIG_MODE_SINGLE)

My question is that, looking at Ads1115.cs for example, it seems that each 'group' of register parameters are abstracted out to their own separate classes, and any parameter that the 'driver' wanted to change is brought in as a local private field or public Property.

My concern is, how does the approach scale to very complicated devices like the uBlox GPS, which can have many registers and hundreds of Register Properties. Sparkfun_Ublox_GPS

In this Device Repo I was expecting to see something similar to the .cpp type libraries where the Registers and Parameters are expressed heavily using enums and bit masking. And a common method of setting registers by themselves or on mass (say during Construction of the device through the constructor)

eg

public void SetConfigParm(ConfigParams param, ConfigParamMasks mask)
        {
            _config ^= (ConfigParams)((ushort)param & (ushort)mask);
        }

        public ConfigParams GetConfigParam(ConfigParamMasks mask)
        {
            return (ConfigParams)((ushort)this._config & (ushort)mask);
        }
 public enum ConfigParams : ushort
    {
        ADS1015_REG_CONFIG_OS_SINGLE = (0x8000),  // Write: Set to start a single-conversion
        ADS1015_REG_CONFIG_OS_BUSY = (0x0000),  // Read: Bit = 0 when conversion is in progress
        ADS1015_REG_CONFIG_OS_NOTBUSY = (0x8000),  // Read: Bit = 1 when device is not performing a conversion

        ADS1015_REG_CONFIG_MUX_DIFF_0_1 = (0x0000),  // Differential P = AIN0, N = AIN1 (default)
        ADS1015_REG_CONFIG_MUX_DIFF_0_3 = (0x1000),  // Differential P = AIN0, N = AIN3
        ADS1015_REG_CONFIG_MUX_DIFF_1_3 = (0x2000),  // Differential P = AIN1, N = AIN3
        ADS1015_REG_CONFIG_MUX_DIFF_2_3 = (0x3000),  // Differential P = AIN2, N = AIN3
        ADS1015_REG_CONFIG_MUX_SINGLE_0 = (0x4000),  // Single-ended AIN0
        ADS1015_REG_CONFIG_MUX_SINGLE_1 = (0x5000),  // Single-ended AIN1
        ADS1015_REG_CONFIG_MUX_SINGLE_2 = (0x6000),  // Single-ended AIN2
        ADS1015_REG_CONFIG_MUX_SINGLE_3 = (0x7000),  // Single-ended AIN3
 public enum ConfigParamMasks : ushort
    {
        ADS1015_REG_CONFIG_OS_MASK = (0x8000),
        ADS1015_REG_CONFIG_MUX_MASK = (0x7000),
        ADS1015_REG_CONFIG_PGA_MASK = (0x0E00),
        ADS1015_REG_CONFIG_MODE_MASK = (0x0100),
        ADS1015_REG_CONFIG_DR_MASK = (0x00E0),
        ADS1015_REG_CONFIG_CMODE_MASK = (0x0010),
        ADS1015_REG_CONFIG_CPOL_MASK = (0x0008),
        ADS1015_REG_CONFIG_CLAT_MASK = (0x0004),
        ADS1015_REG_CONFIG_CQUE_MASK = (0x0003),
    }

Is this type of approach not acceptable?, as that's basically how i've approached writing a device. Hope you get the gist of what I'm trying to get across!

Regards

shaggygi commented 5 years ago

@PRIMETSS I'm not sure I completely understand your question, but have a look at Mcp23xxx binding. It includes a register enum. We don't have a strict standard for structuring commands, registers, etc. and is usually up to the implementer.

Most cases I see the device being instantiated with the communication protocol (I2C, SPI) and a few control pins (if needed). It then would call a method to initialize for setting up registers, pin configurations, etc..

Most cases enums are good enough, but you can also have register classes that have properties (bools, enums, ints, etc.). I have a PR in the pipeline that might help. See CanCtrl.cs and IRegister.cs.

Hope this helps.

krwq commented 5 years ago

Actually @shaggygi has created something most resembling guidelines by adding a template: https://github.com/dotnet/iot/blob/master/tools/templates/DeviceBindingTemplate/README.md

most typically devices have single enum for Registers and some kind of Write/Read which take register and Span/ReadOnlySpan.

I generally don't recommend using too many classes in the design but we also don't forbid them. Long term I'm expecting register classes to be converted to enums/structs.

PRIMETSS commented 5 years ago

Thank-you!

I'll submit the PR later today and see how we go. Just conscious of wasting reviewers time. But sounds like it might be ok!

(ok to close)

krwq commented 5 years ago

@PRIMETSS feel free to contribute any guidelines or anything else you might find useful for the first time experience (it's too late for us to have that perspective anymore) - please let us know if you find any issues. Thanks for interest in contributing!