I've kept this as 3 separate commits to make it easier to review. All 32 spinlock registers are the same, so make them an array to make them easier to access and make the PAC smaller. The spinlock state register (SPINLOCK_ST) should be unmodified by this change.

I don't know why the svd only contains the timers here as triggers, but this is the full enum which can also be used in general in hals. This can e.g. replace/improve the dreq_value function in the rp2040 hal

Please wait for #66 to land before merging this. I think we should leave #61 until after release, we need to resolve how we're fixing the registers that are incorrectly labelled as clear on write. And also some objects now need parens for some reason.

Resolves #65

I wanted to patch the SVDs to make some registers RW instead of RO, but could not test my changes as update.sh would only give me the following output, where svd patch crashes - note that this is WITHOUT any changes to the .yaml:

+ cargo install --version 0.19.0 svd2rust
     Ignored package `svd2rust v0.19.0` is already installed, use --force to override
+ cargo install --version 0.7.0 form
     Ignored package `form v0.7.0` is already installed, use --force to override
+ rustup component add rustfmt
info: component 'rustfmt' for target 'x86_64-unknown-linux-gnu' is up to date
+ pip3 install --upgrade --user 'svdtools>=0.1.15'
Requirement already satisfied: svdtools>=0.1.15 in /home/gottsch/.local/lib/python3.9/site-packages (0.1.18)
WARNING: Keyring is skipped due to an exception: Failed to unlock the keyring!
Requirement already satisfied: lxml~=4.6 in /usr/lib64/python3.9/site-packages (from svdtools>=0.1.15) (4.6.3)
Requirement already satisfied: click~=8.0 in /home/gottsch/.local/lib/python3.9/site-packages (from svdtools>=0.1.15) (8.0.1)
Requirement already satisfied: braceexpand~=0.1.7 in /home/gottsch/.local/lib/python3.9/site-packages (from svdtools>=0.1.15) (0.1.7)
Requirement already satisfied: PyYAML~=5.3 in /usr/lib64/python3.9/site-packages (from svdtools>=0.1.15) (5.4.1)
+ rm -rf src
+ mkdir src
+ svd patch svd/rp2040.yaml
Traceback (most recent call last):
  File "/home/gottsch/.local/bin/svd", line 8, in <module>
    sys.exit(svdtools_cli())
  File "/home/gottsch/.local/lib/python3.9/site-packages/click/core.py", line 1137, in __call__
    return self.main(*args, **kwargs)
  File "/home/gottsch/.local/lib/python3.9/site-packages/click/core.py", line 1062, in main
    rv = self.invoke(ctx)
  File "/home/gottsch/.local/lib/python3.9/site-packages/click/core.py", line 1668, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/home/gottsch/.local/lib/python3.9/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/gottsch/.local/lib/python3.9/site-packages/click/core.py", line 763, in invoke
    return __callback(*args, **kwargs)
  File "/home/gottsch/.local/lib/python3.9/site-packages/svdtools/cli.py", line 16, in patch
    svdtools.patch.main(yaml_file)
  File "/home/gottsch/.local/lib/python3.9/site-packages/svdtools/patch.py", line 1472, in main
    process_device(svd, root)
  File "/home/gottsch/.local/lib/python3.9/site-packages/svdtools/patch.py", line 1449, in process_device
    d.process_peripheral(periphspec, device[periphspec], update_fields)
  File "/home/gottsch/.local/lib/python3.9/site-packages/svdtools/patch.py", line 670, in process_peripheral
    p.process_register(rspec, register, update_fields)
  File "/home/gottsch/.local/lib/python3.9/site-packages/svdtools/patch.py", line 1050, in process_register
    r.process_field(pname, fspec, field)
  File "/home/gottsch/.local/lib/python3.9/site-packages/svdtools/patch.py", line 1298, in process_field
    self.process_field_enum(pname, fspec, field)
  File "/home/gottsch/.local/lib/python3.9/site-packages/svdtools/patch.py", line 1316, in process_field_enum
    for ftag in sorted_fields(list(self.iter_fields(fspec))):
  File "/home/gottsch/.local/lib/python3.9/site-packages/svdtools/patch.py", line 1060, in sorted_fields
    return sorted(fields, key=lambda ftag: int(ftag.find("bitOffset").text, 0))
  File "/home/gottsch/.local/lib/python3.9/site-packages/svdtools/patch.py", line 1060, in <lambda>
    return sorted(fields, key=lambda ftag: int(ftag.find("bitOffset").text, 0))
AttributeError: 'NoneType' object has no attribute 'text'

Note that the keyring error early in the script does not really make sense given that pip purely installs dependencies in my home directory, but it should not have any negative impact, either.

Given that there are no modifications reported by git except those caused by update.sh, I would have expected the script to execute correctly.

Hi,

Having bought a Pi Pico recently and wanting to try my hand on bare-metal/embedded/low-level Rust, I've been toying with this crate for the last 3 weeks or so.

I've started from the pico-blink-rs example and then I built HALs for GPIOs, XOSC, PLLs and Clocks. I got GPIOs to work fine. I then put my focus on PLLs, my end goal being to setup clocks in order to setup UART.

To do that I replicated in Rust, using this crate, all the steps outlined in the official C code from the pico-sdk.

For the last week or so now I've been scratching my head because PLLs wouldn't work at all. In order to debug I built a 7-Leds array with a Serial-to-Parallel shift register to display some values as binary. Thanks to bit shifting and a small delay I can display values 7 bits by 7 bits. The 8th LED is used as status LED. I started debugging frequencies first with the Frequency Counter, and that would show 0Hz for my otherwise, in theory (reading the code) well configured PLL. Then I started debugging values for divders (refdiv, fbdiv, post div 1, post div 2); all of whom have correct values. I went on with the PWR register, showing 1s in the wrong places, despite this code being written and called :+1: ``

        // Turn on self.device
        self.device.pwr.write(|w| unsafe {
            w.pd().clear_bit();
            w.vcopd().clear_bit();
            w
        });
        /* .... */
        self.device.pwr.write(|w| unsafe {
            w.postdivpd().clear_bit();
            w
        });

I then replaced those clear_bit() calls with a blanket w.bits(0) and suddenly my PLL turned on, showing a nominal 125Mhz. So, either I'm wrong to think that clear_bit() is supposed to set a bit to 0, or it does not work... or something else is wrong. I now wonder if my struggle with clocks isn't also linked to the same kind of issue.

Any thoughts ?

There have been some useful PRs since 0.3.0 release. In particular, I want access to the software interrupts. I'm using a local version of rp2040_hal to specify the latest pac from Github as dependency.

Can we bump the version to 0.3.1 and publish? I can work on a PR, but I don't know if there is a game plan.

Thanks!

Only functional change in this PR is updating cortex-m Other than that, just bumping the pac version number and updating release notes. I wanted to update the build tools (svd2rust, form) but doing so will require some fix-ups on rp-hal at the same time, so it makes sense to depend on a release version first.

According to 2.19.2 from the datasheet each pin has its own set of functions. This crate generates set functions for each function but each pin seems to have the same set of set functions (which seem to match gpio pin 0)

I expect to be able to do something like:

    p.IO_BANK0.gpio[16].gpio_ctrl.modify(|_,w|w.funcsel().spi0_rx());
    p.IO_BANK0.gpio[17].gpio_ctrl.modify(|_,w|w.funcsel().spi0_csn());
    p.IO_BANK0.gpio[18].gpio_ctrl.modify(|_,w|w.funcsel().spi0_sck());
    p.IO_BANK0.gpio[19].gpio_ctrl.modify(|_,w|w.funcsel().spi0_tx());

but all pins only have spi0_rx() for spi0 functions (which is wrong)

There is new pico sdk release, and it contains important changes (eg. required for usb support)

https://github.com/raspberrypi/pico-sdk/commit/0e95cbd25713a72d173f29ddaa7ead69deee0e87

Can we rebuild files basing on new svd? Also this repository contains patched off sync patched version that is used for generation, can we maybe maintain instead .patch file or script for patching, so anyone could repatch it from original svd file? Or if that is too complex or just too temporary, could we at least document changes that are needed.

thanks

In reference manual section 2.3.1.7, they describe SPINLOCK0-31 as being read only. But, in the description (page 60), they state:

Reading from a spinlock address will:
- Return 0 if lock is already locked
- Otherwise return nonzero, and simultaneously claim the lock
Writing (any value) releases the lock.

So they're not really read-only registers - they are read-write! We should patch the SVD to reflect this.

I propose a push-early, push-often approach to this one. I can't see any harm in at least putting out a 0.1.0 release, so that crates can starting depending on it.

I wrote this out in matrix a week ago. I've updated it to remove some inaccuracy, and am recording it here so we don't forget to fix it.

I've tested out using the Rust svdtools generated PAC - looks like we might need some changes around some registers. Specifically, the SVD describes some fields as modifiedWriteValues: clear from the SVD reference: an example of one of these fields from the rp2040 reference manual

we were previously using .set_bit() on these fields, but in svd2rust this is removed for modifiedWriteValues: clear (since as far as it's concerned, any operation will clear the bit)

the accessor still has a .bit(value:bool) associated function, so we could change all of our calls on these frields from .set_bit() to .bit(true) and it should work for both old and new versions of the PAC

I feel like the SVD has probably got it wrong - at least in the case above. the write doesn't clear the bitfield. it sets the bit, and then you have to poll it until it's clear before moving on - it's a later hardware event that clears the bitfield.

When I did the release I forgot to update the links to the diffs at the bottom of CHANGELOG.md When this file gets updated next, update the links as below

[Unreleased]: https://github.com/rp-rs/rp2040-pac/compare/v0.4.0...HEAD
[0.4.0]: https://github.com/rp-rs/rp2040-pac/compare/v0.3.0...v0.4.0

Current versions of svdtools (0.1.24 and 0.1.25) don't work with this pac.

0.1.25 fails with TypeError: Argument must be bytes or unicode, got 'int'. This should be fixed by https://github.com/stm32-rs/svdtools/pull/127. However, with that fix, we get another error: https://github.com/stm32-rs/svdtools/runs/8293034774?check_suite_focus=true

[ERROR svd2rust] Error rendering device
    
    Caused by:
        0: Rendering error at peripheral
           Name: DMA
           Description: DMA with separate read and write masters
           Group: No group name
        1: register CH0_CTRL_TRIG not found

I think this is caused by some interaction between _cluster and other patches on the same element in rp2040.yaml. Didn't look into it in detail, yet.

Please forgive me, if this is stupid, but I get a little confused by the nitty-gritty details of the move and borrow semantics of the SVD HALs.

The 12 DMA channels can be found in the DMA.ch array, but if I'm not wrong, this prevents a certain peripheral from taking ownership of a single channel:

let mut pac = pac::Peripherals::take().unwrap();
let mut chan0 = pac.DMA.ch[0];

error[E0507]: cannot move out of dereference of `DMA`
   --> src/main.rs:143:21
    |
143 |     let mut chan0 = pac.DMA.ch[0];
    |                     ^^^^^^^^^^^^^
    |                     |
    |                     move occurs because value has type `rp2040_hal::rp2040_pac::dma::CH
`, which does not implement the `Copy` trait
    |                     help: consider borrowing here: `&pac.DMA.ch[0]`

One could pass the whole DMA struct to please the borrow checker, but that obviously prevents the independent use of channels. I'd suggest to cluster the 12 channels in separate modules.

I had some code that resembled this:

        for smnum in 0..STRAND_COUNT {
            // irrelevant stuff omitted
            p.DMA.ch[smnum].ch_al1_ctrl.write(|w| unsafe {
                w.read_error().set_bit() // to clear
                    .write_error().set_bit() // to clear
                    .bswap().set_bit()
                    .treq_sel().bits(0 + smnum as u8) // DREQ_PIO0_TX0..3 are contiguous
                    .incr_write().clear_bit()
                    .incr_read().set_bit()
                    .data_size().size_word()
                    .high_priority().set_bit()
                    .en().set_bit()
            });
        }

It causes rather elaborate misbehavior on DMA channel 0.

Can you see why?

Unfortunately, you can't see why, as the critical piece of information does not actually appear in the code. svd2rust defaults any register bitfields that are not explicitly set to its idea of the reset value (a design flaw, imo, but hey). The rp2040 SVD from the RaspPi folx have all the DMA channel registers modeled separately, which is good, because they have different reset values.

Why? Because for the CHAIN_TO field to be a no-op, it has to be set to the ID of the channel being configured. So for channel 0 it's 0, for channel 1, it's 1, etc.

The PAC loses this information from the SVD by throwing away the types of channels 1-7 and using the channel 0 type for everything. Of course, you have to do that to make an array. (svd2rust also makes types distinct just so they can have different reset values, preventing them from being in an array, which, well, you can guess my opinion on that.)

The net effect of this is that every channel other than 0 implicitly triggers channel 0 when it completes. Every time.

I'm not actually sure how to fix this. Moving away from the array would be terribly unfortunate and cause me to basically hack an array back in so that I can loop over channels (god forbid you wanted to allocate channels dynamically, with them all becoming different types!). However, the current situation is a rather elaborate footgun for DMA. I don't believe svd2rust has any notion of a field that must be set explicitly when writing a register, which otherwise would be a reasonable thing to reach for here.