Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    -8 -0.002% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.38%. Comparing base (4ce2dd2) to head (aeec699).
⚠️ Report is 13 commits behind head on master.

@@           Coverage Diff           @@
##           master   #17874   +/-   ##
=======================================
  Coverage   98.38%   98.38%           
=======================================
  Files         171      171           
  Lines       22283    22295   +12     
=======================================
+ Hits        21924    21936   +12     
  Misses        359      359           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

CI windows port / build-vs (x64, Debug, dev, 2017) failure is unrelated.

Hi @robert-hh,
I would love to hear what you think.
Thanks,

Thank you for the PR: From what I understand, it selects the input B of an ADC if it exists. Doing that, if actually fixes a bug, which caused A1, A3 and A4 not to return ADC values at the MIMXRT1170_EVK board. Thanks.
My comments on this change.

• Since it selects the channel_sample_mode, maybe call it as such, or shorter channel_mode, matching the NXP header file terminology. The data sheet calls it channel_input, but since it's more that just the input, the NXP header file uses a better term.
• The mode is not configurable by the user. Then it should not be visible when the ADC properties are printed in mp_machine_adc_print()
• If other than the A0-A5 pins are available for ADC, then it should be documented, like in the pin assignment document. For the moment the documentation just assumes support at the pins that are listed as such in the vendor's board pinout. Edit: It seems that every GPIO_AD_XX pin supports analog input.

If you want to go on another step ahead, you could make the channel_mode configurable, allowing the differential mode. Since that requires to pins to be specified, either these pins have to be named directly, of one would assume sequential pin numbers. Like for the MIMXRT1170_EVK board "A0" in differential mode would specify "A0" and "A1" as paired input. For that board, the A0/1, A2/3 and A4/5 make up a differential pair.
But maybe that's too much of a change for the moment, and it would require additional documentation.
I do not know if differential input is supported by any port, and even in this port it would apply only to the MIMXRT117x series, making it's acceptance less probable.

Helllo @robert-hh ,

Thank you for reviewing.

• Since it selects the channel_sample_mode, maybe call it as such, or shorter channel_mode, matching the NXP header file terminology. The data sheet calls it channel_input, but since it's more that just the input, the NXP header file uses a better term.

I tried to add this, but then I need to #if defined(MIMXRT117x_SERIES) also in make_new() and in the _machine_adc_obj_t, which made implementation less readable.

I think best to calculate the mode at the already platform specific read_u16().

We specify the group not the mode. As you write next, we may have several modes, single-ended, diff-AB, diff-BA, we hold the group and calculate the mode based on the requested mode and group at the read_u16(), currently supporting only the single-ended.

• The mode is not configurable by the user. Then it should not be visible when the ADC properties are printed in mp_machine_adc_print()

I am confused...

            mp_printf(print, "ADC(%u, channel=%u, channel_group=%u)", i, self->channel, self->channel_group);

None of the instance, channel and channel_group are configurable, all obtained from the architecture pins database, and all are helpful to understand what the object is operating on.

This information was helpful for me when debugging this issue, I added the channel_group for consistency, aka instance/channel/group are derived from the pin database.

Should I remove all?

Please notice this is not the mode, which is hardcoded to single-ended in all cases and unrelated to this.

* If other than the A0-A5 pins are available for ADC, then it should be documented, like in the pin assignment document. For the moment the documentation just assumes support at the pins that are listed as such in the vendor's board pinout. Edit: It seems that every GPIO_AD_XX pin supports analog input.

I do not understand where you want to document this can you please provide a reference?

Since kLPADC_SampleChannelSingleEndSideA has the value of 0 and kLPADC_SampleChannelSingleEndSideB the value of 1, you could directly assign self->channel_group (or whatever it will be called) to adc_config.sampleChannelMode. As long of course as the implementation supports single ended mode only.

I do not like to mix oracle values of one type with another type just because the oracle is the same. And as you write later, in future we will have different modes for the same group and we need to differentiate the group, mode and the result. I left it as-is, please let me know if you want to merge oracles, but when adding diff-AB, diff-BA we will need to avoid using oracles again.

If you want to go on another step ahead, you could make the channel_mode configurable, allowing the differential mode. Since that requires to pins to be specified, either these pins have to be named directly, of one would assume sequential pin numbers. Like for the MIMXRT1170_EVK board "A0" in differential mode would specify "A0" and "A1" as paired input. For that board, the A0/1, A2/3 and A4/5 make up a differential pair. But maybe that's too much of a change for the moment, and it would require additional documentation. I do not know if differential input is supported by any port, and even in this port it would apply only to the MIMXRT117x series, making it's acceptance less probable.

You are correct in the fact that there is no actual choice of pins, there is no reason to allow to select the "other" pin, the leading pin (A) will be provided and the pair (B) will be selected automatically. BTW: I think you got this wrong, the pairs are A0/B0 and A1/B1, etc.

This patch may be applied in future on top by adding a constructor mode parameter deriving the sample mode from the self->mode and self->channel_group, it can be done later.

Based on the above response, I have not made any change in the patchset... please let me know if you agree to the analysis. I can try and work on a differential patch on top later.

Regards,

About channel_group: Forget my comment. That's a term from the MIMXRT_10xx family's library. So just keep it. You use the data element channel_group from the machine_adc_obj_t to select the input. In the MIMXRT10xx this was to select between ADC1 and ADC2, where ADC2 was never used.

About mp_machine_adc_print(): At the moment it prints the instance and the channel, which is already inconsistent because the channel cannot be chosen by the user. For consistency then the channel should be removed. The aim for MicroPython is, that the printout of an object matches the argument list for instantiation. But there are ATM more cases of deviation than match.

I do not understand where you want to document this can you please provide a reference?

There is no good place for adding such hint. Maybe a short note in the quickref.rst, that besides the pins labelled as ADC other pins from the GPIO_AD_XX block can be used for ADC. But that was already true before.

I do not like to mix oracle values of one type with another type just because the oracle is the same.

I understand your concern, and the MIXMRT11xx boards are not low at flash. SO just keep it.

You are correct in the fact that there is no actual choice of pins, there is no reason to allow to select the "other" pin, the leading pin (A) will be provided and the pair (B) will be selected automatically. BTW: I think you got this wrong, the pairs are A0/B0 and A1/B1, etc.

With respect to Pin names, we seem to talk about different things. You talk about A/B channels of a ADC input, I talked about the Pin labels was written on the silkscreen of a given board. These names are used in the user documentation. It is True that at ADC hardware level the (B) input follows the (A) input, but the related board pins are not necessary adjacent pins. In any case such pin pairs at a board have to be documented. But at least it seems that a certain ADC#/CH#/INPUT# triple is always assigned to a single GPIO. Looking through the mimxrt1176_af.csv I noticed, that at some GPIO pins channels from both ADC1 and ADC2 are assigned. I should remember since I created that file, but it's a while since then.

Hello @robert-hh ,

Reading your verbose response, do I understand correctly that you want to remove all the content of:

            mp_printf(print, "ADC(%u, channel=%u, channel_group=%u)", i, self->channel, self->channel_group);

As all not provided? not only channel but also the ADC instance? or leave only instance?

            mp_printf(print, "ADC(%u);

Any other change you would like to see?

Thanks,

Hi,

I tried and failed to create a patch to add differential mode.

Adding enum:

enum {
    ADC_MODE_SINGLE_ENDED = 0,
    ADC_MODE_DIFFRENCIAL_AB,
    ADC_MODE_DIFFRENCIAL_BA,
};  

And keyword argument mode=ADC_MODE_SINGLE_ENDED to the constructor, however, I failed as my knowledge of micropython is limited.

Not sure how to add this:

static const mp_rom_map_elem_t machine_adc_locals_dict_table[] = {
    // class constants
    { MP_ROM_QSTR(MP_QSTR_ADC_MODE_SINGLE_ENDED),   MP_ROM_INT(ADC_MODE_SINGLE_ENDED) },
    { MP_ROM_QSTR(MP_QSTR_ADC_MODE_DIFFRENCIAL_AB), MP_ROM_INT(ADC_MODE_DIFFRENCIAL_AB) },
    { MP_ROM_QSTR(MP_QSTR_ADC_MODE_DIFFRENCIAL_BA), MP_ROM_INT(ADC_MODE_DIFFRENCIAL_BA) },
};
static MP_DEFINE_CONST_DICT(machine_adc_locals_dict, machine_adc_locals_dict_table);

As it conflicts with the extmod/machine_adc.c.

Let's merge this work first then I will try to figure out how to do this.

Thanks,

As all not provided? not only channel but also the ADC instance? or leave only instance?

I'm not sure about what to recommend. Looking at the real output, nothing of that matches the constructor inputs. So we could keep it for now. Only for MIMXRT11x I would change the text from "channel_group" to "channel input"

Another thing I forgot to mention:

In your PR you use the NXP library version 2.16 from @Hatach 's repository. The version used by MicroPython is actuall V2.11, with the git hash fa5a554c7944d2a196626f8d3631e44943f9abcc. There is an open PR 16235 for an update to v2.16 which is somehow stuck. From my test it seems fine to do make the change, but please as a separate commit. For this commit please use v2.11, unless you have a strong reason to use v2.16.

I tried and failed to create a patch to add differential mode.

Better put the differential mode into a separate PR and keep this PR as a bug fix. Then chances are good that it will get merged fast. Since differential mode is not a common feature, it may stick in PR the queue for a very long time.

Not sure how to add this:

You have to define MICROPY_PY_MACHINE_ADC_CLASS_CONSTANTS in line 42 of machine_adc.c e.g. as:

#define MICROPY_PY_MACHINE_ADC_CLASS_CONSTANTS \
    { MP_ROM_QSTR(MP_QSTR_ADC_MODE_SINGLE_ENDED),   MP_ROM_INT(ADC_MODE_SINGLE_ENDED) }, \
    { MP_ROM_QSTR(MP_QSTR_ADC_MODE_DIFFERENTIAL_AB), MP_ROM_INT(ADC_MODE_DIFFERENTIAL_AB) } \,
    { MP_ROM_QSTR(MP_QSTR_ADC_MODE_DIFFERENTIAL_BA), MP_ROM_INT(ADC_MODE_DIFFERENTIAL_BA) }, 

Then if will be added to the modules dictionary table of micropython/extmod/machine_adc.c, which includes the port's machine_adc.c file. I would suggest supporting just one of the differential modes, as the results only differ by the sign, and shorten the names, maybe to "SINGLE_ENDED" and "DIFFERENTIAL". The ADC part is obvious, since these are constants of the ADC module.

But as said, keep that for later.

Only for MIMXRT11x I would change the text from "channel_group" to "channel input"

Done, I hope this what you meant.

In your PR you use the NXP library version 2.16

Thank you for capturing this, it was a huge mistake that was added into this patchset.

Looks better now. Looks like I made a typo for print. Of course it should be channel_input in one word.
Besides that:
Tested with a MIMXRT1170_EVK board and a singe AA battery as analog voltage source. All inputs A0 - A5 return a proper value, with read_uv() returning values in the range of 1_600_000 (the voltage meters return 1.589V).

Looks better now. Looks like I made a typo for print. Of course it should be channel_input in one word.

Fixed.

Thank you!

Differential mode would also have to cater for the different type of the ADC result, which is 13 bits signed instead of 12 bits unsigned. Since read_u16() returns an unsigned quantity, the read() method may have to be used to return the signed value. ADC.read() is port-specific and returns the raw ADC value.