Thank you for the contribution. A few questions to the DAC code.

• at first, thank you for reminding that practically the dac_init flag was not used. Your change makes proper use of that flag, which is now a pair.
• I see that you disable DAC before the clock is applied. I'm not sure if setting registers of the DAC works without a clock. So the first thing I did usually was enabling the clock. And I had not the impression that disable is required, because reset disables the DAC as well, and enabling an enabled DAC is a NOP. Alternatively, you can check the enable flag before enabling the DAC.

The pins.csv looks like a pins.csv for the board which I made recently, without making a PR. Beside some pin naming differences, I added the following pin names as well:

PIN_PB11,QSPI_CS
PIN_PB10,QSPI_SCK
PIN_PA08,QSPI_D0
PIN_PA09,QSPI_D1
PIN_PA10,QSPI_D2
PIN_PA11,QSPI_D3

With these, the flash chip is easier accessible for a Python based flash driver (see e.g. https://github.com/robert-hh/SPI_Flash), and the PR for using the on board flash will uses these.

You mentioned changes to the RTC module, but did not include them.

Another note:
You might not need the line:
#define MICROPY_HW_MCU_OSC32KULP (1)
This is a special workaround for PCB layout problems of some Adafruit boards. Without that line, the CPU clock and with that systick is more precise.

About the PR structure: You could combine the commits of this PR into one with the board files and one with the DAC change, or make two PRs out of it.

The pins.csv looks like a pins.csv for the board which I made recently, without making a PR. Beside some pin naming differences, I added the following pin names as well:

PIN_PB11,QSPI_CS
PIN_PB10,QSPI_SCK
PIN_PA08,QSPI_D0
PIN_PA09,QSPI_D1
PIN_PA10,QSPI_D2
PIN_PA11,QSPI_D3

With these, the flash chip is easier accessible for a Python based flash driver (see e.g. https://github.com/robert-hh/SPI_Flash), and the PR for using the on board flash will uses these.

Added - will rewrite the change set as suggested once other points you raised addressed.

• I see that you disable DAC before the clock is applied. I'm not sure if setting registers of the DAC works without a clock. So the first thing I did usually was enabling the clock. And I had not the impression that disable is required, because reset disables the DAC as well, and enabling an enabled DAC is a NOP. Alternatively, you can check the enable flag before enabling the DAC.

§47.6.2.3 of the SAMD5x family data sheet states

DAC Configuration

Each individual DAC is configured by its respective DAC Control register (DACCTRLx)). These settings are applied when DAC Controller is enabled and can be changed only when DAC Controller is disabled.

I recall I also found that the second dac would not be correctly configured while leaving the first configured if I did not disable the system when updating the registers.

You mentioned changes to the RTC module, but did not include them.

This was something of a discovery process for me (first time I have ported micropython) and this change related to adding these defines

+#define MICROPY_HW_XOSC32K          (1)
+#define MICROPY_HW_MCU_OSC32KULP    (1)

which you make further reference to and I need to investigate some more...

About the PR structure: You could combine the commits of this PR into one with the board files and one with the DAC change, or make two PRs out of it.

Rewrote, but got distracted by trying to satisfy the formatting checks and realised that the changes are not split quite the way you suggested. Will come back to this later... many thanks for your suggestions.

About the PR structure: You could combine the commits of this PR into one with the board files and one with the DAC change, or make two PRs out of it.

Rewrote, but got distracted by trying to satisfy the formatting checks and realised that the changes are not split quite the way you suggested. Will come back to this later... many thanks for your suggestions.

Now rewritten...

Thank you for the changes.

I recall I also found that the second dac would not be correctly configured while leaving the first configured if I did not disable the system when updating the registers.

That's fine. At first glance I did not notice, that the path for if (!dac_init[self->id]) can be executed even if the path if (!(dac_init[0] | dac_init[1])) { was not, and then DAC is not disabled. My major concern was accessing the DAC before the bus clock in the APBD register is enabled. As said, I do not recall the behavior for the DAC peripheral, but in my test a non-enabled bus clock was often the reason for code lock-up. You should be able to move DAC disable into the if (!dac_init[self->id]) block.

You added init value for vref into the static DAC structure. That's fine. But you used numeric constants, instead of the DEFAULT_DAC_VREF defines. Using the latter requires to shift around the code blocks a little bit.

About manifest.py: Do not change manifest.py at the boards directory. These would apply to all boards, even if they do not have the respective hardware enabled of sufficient free code space. If you want to add additional files for a board, add a manifest.py file to the board's directory, and add a line:
FROZEN_MANIFEST = $(BOARD_DIR)/manifest.py
to mpconfigboard.mk of the board.

Another note: If you want to use the SDCARD using the sdcard driver in https://github.com/micropython/micropython-lib/tree/c8603192d1d142fdeb7f9578e000c9834669a082/micropython/drivers/storage/sdcard, It is useful to have the following lines in mpconfigboard.h:

// fatfs configuration used in ffconf.h
#define MICROPY_FATFS_ENABLE_LFN            (1)
#define MICROPY_FATFS_RPATH                 (2)
#define MICROPY_FATFS_MAX_SS                (4096)
#define MICROPY_FATFS_LFN_CODE_PAGE         437 /* 1=SFN/ANSI 437=LFN/U.S.(OEM) */

About DAC disable before enabling the APBD clock. That works, but maybe by chance. I looked at it with a debugger.
After reset, with the APBD clock for DAC disabled, both the CTRLA and SYNCBUSY register show a value of 0. So the first disable will pass, even if the DAC would not respond to a setting. Probably, trying to enable DAC without a APBD clock would fail. At least that's what happens when I try it with the debugger.

About manifest.py: Do not change manifest.py at the boards directory. These would apply to all boards, even if they do not have the respective hardware enabled of sufficient free code space. If you want to add additional files for a board, add a manifest.py file to the board's directory, and add a line: FROZEN_MANIFEST = $(BOARD_DIR)/manifest.py to mpconfigboard.mk of the board.

Done, was not aware of convention

Will have to get to remaining suggestions later, but thank you

You added init value for vref into the static DAC structure. That's fine. But you used numeric constants, instead of the DEFAULT_DAC_VREF defines. Using the latter requires to shift around the code blocks a little bit.

Removed init value in preference to the broader re-arrangement

About DAC disable before enabling the APBD clock. That works, but maybe by chance. I looked at it with a debugger. After reset, with the APBD clock for DAC disabled, both the CTRLA and SYNCBUSY register show a value of 0. So the first disable will pass, even if the DAC would not respond to a setting. Probably, trying to enable DAC without a APBD clock would fail. At least that's what happens when I try it with the debugger.

Understood, change incoming - the "works by chance" is probably why I had not stumbled over this

Just by way of some commentary:

from machine import DAC

dac0 = DAC(0)
dac1 = DAC(1)

for j in range(0, 0x1000, 0x10):
    dac0.write(j)
    dac1.write(j)

del(dac0)
del(dac1)

dac0 = DAC(0)
dac1 = DAC(1)

for j in range(0, 0x1000, 0x10):
    dac0.write(j)
    dac1.write(j)

del(dac0)
del(dac1)

dac0 = DAC(0)
dac1 = DAC(1)

while 1:
    for j in range(0, 0x1000, 0x10):
        dac0.write(j)
        dac1.write(j)

Seems to not lock up and generate the (rather jittery...) sawtooth one may expect

generate the (rather jittery...) sawtooth one may expect

That looks fine. For repeating DAC output and ADC sampling, you may as well have a look at PR #9624, which implements a timed DAC.write ADC.read using DMA and a timer.

generate the (rather jittery...) sawtooth one may expect

That looks fine. For repeating DAC output and ADC sampling, you may as well have a look at PR #9624, which implements a timed DAC.write ADC.read using DMA and a timer.

That's in my queue - I have done a fair chunk of register bashing on the rp2040 with assembly and machine.mem32 to do chained DMA etc. with a resistor DAC - part of my motivation of picking up the Grand Central was for a decent hardware DAC.

Your PR mentioned above looks exactly like what was next on my to-do by more register bashing 🙂 thank you

The PR looks fine now and the DAC code makes just the minimal changes. And it is good that you can confirm it works. I had made board files for the board before for another user, but lacking a board I could not test them, and I did not get any feedback. I cannot tell if and how long it takes for the PR to get merged. Sometimes it's days, sometimes months, sometimes never. But independent form that PR are a good source of information for all of us.

You did not mention a picture of the board in the json file. A picture is used for the download page. Adding it requires another PR at the micropython-media repository at https://github.com/micropython/micropython-media. The name of the picture file and the directory name of the board must be the same in both repositories. See also the other boards as example.

If you have time and since you have the board, you could try PR #10233, which uses an on-board external flash chip for the file system. You would have to specify the flash chip in mpconfigboard.h, with a line:

#define MICROPY_HW_QSPIFLASH GD25Q64C

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.57%. Comparing base (b08ddbb) to head (b495a46).

@@           Coverage Diff           @@
##           master   #11104   +/-   ##
=======================================
  Coverage   98.57%   98.57%           
=======================================
  Files         164      164           
  Lines       21341    21341           
=======================================
  Hits        21036    21036           
  Misses        305      305           

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

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:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:   +52 +0.019% ADAFRUIT_ITSYBITSY_M4_EXPRESS[incl +4(bss)]

This PR seems to be messed up, looking at the number of commits it wants to include. First step should be to rebase your branch on to the current master. Or better close it and restart with the board files based on the current master.
The format of the pins.csv file has been changed. The columns are swapped, and at the port ID the PIN_ prefix is dropped, e.g. A0,PA02. Attached is a sample version of a new pin.csv. The changes to DAC are still fine. I'm just surprised that they did not make it into the master. I recall that they did. Anyhow, better make DAC change and the new board separate PRs.

pins.zip

Closing in favour of a rewritten PR