Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Dac1/Dac2 drivers into a single Dac driver #1661

Merged
merged 6 commits into from
Jun 10, 2024

Conversation

jessebraham
Copy link
Member

Opening as draft as this is still completely untested and I don't know if it actually works or not yet. It should in theory.

Closes #1472.

@burrbull
Copy link
Contributor

burrbull commented Jun 5, 2024

Indexes start from 0

@jessebraham
Copy link
Member Author

jessebraham commented Jun 5, 2024

The documentation generated for the PAC says otherwise. I don't know why but svdtools 1-indexed these.

@jessebraham
Copy link
Member Author

Ahh just in the SENS module I guess, it's inconsistent for whatever reason.

@bjoernQ
Copy link
Contributor

bjoernQ commented Jun 6, 2024

I'm getting this

!! A panic occured in 'C:\Users\Bjoern\.cargo\git\checkouts\esp-pacs-175859a47098a821\a7c72f7\esp32\src\rtc_io.rs', at line 129, column 10:
index out of bounds: the len is 2 but the index is 2

Backtrace:

0x400d049d
0x400d049d - esp32::rtc_io::RegisterBlock::pad_dac
    at C:\Users\Bjoern\.cargo\git\checkouts\esp-pacs-175859a47098a821\a7c72f7\esp32\src\rtc_io.rs:129
0x400d41ca
0x400d41ca - Reset
    at C:\Users\Bjoern\.cargo\registry\src\index.crates.io-6f17d22bba15001f\xtensa-lx-rt-0.16.0\src\lib.rs:70
0x400d34f2
0x400d34f2 - ESP32Reset
    at C:\projects\review\jesse\esp-hal\esp-hal\src\soc\esp32\mod.rs:93
0x3ffffffd
0x40079a81
0x400806b5
0x400806b5 - esp_backtrace::arch::backtrace_internal
    at C:\projects\review\jesse\esp-hal\esp-backtrace\src\xtensa.rs:287
0x40007c31
0x4000073d
0x3ffffffd

Adjusting the INDEX to be zero-based (0,1) makes it work

@jessebraham
Copy link
Member Author

jessebraham commented Jun 10, 2024

Adjusting the INDEX to be zero-based (0,1) makes it work

I blame svd2rust, this is really misleading 😁 (It's 1-indexed in the SVD too, which is why I didn't question it)
Screenshot 2024-06-10 at 06 24 01

Will fix shortly

@jessebraham jessebraham marked this pull request as ready for review June 10, 2024 13:28
Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for taking care of this!

Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and my voltmeter agrees 😄

@bjoernQ bjoernQ added this pull request to the merge queue Jun 10, 2024
Merged via the queue into esp-rs:main with commit dc8e0a6 Jun 10, 2024
29 checks passed
@burrbull
Copy link
Contributor

I blame svd2rust, this is really misleading 😁 (It's 1-indexed in the SVD too, which is why I didn't question it)

svd2rust uses 0-based indexing for all arrays. This does not require advanced checks, simpler for calculation and support non-numeric indices. For note, chiptool also uses 0.

There were tries earlier to use values equivalent to indices, but it was bad experience.

P.S. Enter into DAC_CW_EN_R docs. There should be note.

@jessebraham jessebraham deleted the feature/dac branch June 10, 2024 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DAC1 and DAC2 can be merged into one driver
4 participants