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

implement Debug for readable fields of registers #716

Merged
merged 1 commit into from
Jun 2, 2023

Conversation

liebman
Copy link
Contributor

@liebman liebman commented May 16, 2023

contributes towards #48

Notes:

  • I've only run this with esp (esp32/esp32s3) pacs
  • I'd love some feedback on where I've implemented this and if there is a cleaner way

src/generate/register.rs Outdated Show resolved Hide resolved
src/generate/register.rs Outdated Show resolved Hide resolved
src/generate/register.rs Outdated Show resolved Hide resolved
@burrbull
Copy link
Member

I've missed that you targeting esp32. Do I understand right that you don't have other way to debug except this?
IDE extensions like probe-rs or Cortex-m-Debug for VSCode?

@liebman
Copy link
Contributor Author

liebman commented May 19, 2023

Cortex debug can work easily for newer esp32 family chips (esp32s3, esp32c3, etc) that have builtin usb - for those without builtin usb its only available if the JTAG pins are available.

One specific use case here is that many of the peripherals (for me this is RTC_CTRL power management) are not well documented. And since there is a std hal for esp32 that is built on esp-idf one thing I can do is dump peripherals registers on the std hal to help with implementations of the no-std hal by being able to compare what registers are set differently. After manually building this for esp32 I was starting on esp32s3 and realized this would be much better placed in svd2rust and could be automatically generated - thats when I came across #48.

@liebman
Copy link
Contributor Author

liebman commented May 19, 2023

What I really want is to be able to dump all registers from the peripheral level as registers (and fields) differ a fair amount between esp32 chip types. (but at least having the registers handled automatically is a huge help.

@burrbull
Copy link
Member

What I really want is to be able to dump all registers from the peripheral level as registers (and fields) differ a fair amount between esp32 chip types. (but at least having the registers handled automatically is a huge help.

If you want to read a range of registers, you should also decide what to do with registers which have read side effects (see readAction).

@burrbull
Copy link
Member

Don't do merge master. Use rebase instread. We want to have linear commit history.

@liebman
Copy link
Contributor Author

liebman commented May 20, 2023

I'm already skipping registers that have read side effects. Currently not even generating the Debug implementation but maybe I should and just print the register name and no value or something short indicating it was not read.

@liebman
Copy link
Contributor Author

liebman commented May 20, 2023

Don't do merge master. Use rebase instread. We want to have linear commit history.

How can I fix this?

@liebman
Copy link
Contributor Author

liebman commented May 28, 2023

I've added debug at the RegisterBlock level so one can print all registers belonging to a peripheral with:

    info!("SHA {:#?}", unsafe { &*esp::SHA::ptr() });

Write only registers are printed with a note that they are read only and registers that have read actions are also printed with a message. All readable registers have their fields printed in struct format and if there are no fields then the raw register value is printed. Register clusters "should" also be handled.

@liebman liebman marked this pull request as ready for review May 28, 2023 22:40
@liebman liebman requested a review from a team as a code owner May 28, 2023 22:40
burrbull
burrbull previously approved these changes May 30, 2023
@burrbull
Copy link
Member

Looks good for me.
@Emilgardis ?

@burrbull
Copy link
Member

@liebman Could you how some usage example output?

Emilgardis
Emilgardis previously approved these changes May 30, 2023
Copy link
Member

@Emilgardis Emilgardis left a comment

Choose a reason for hiding this comment

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

this is good as is, just some improvements on naming and suggestion for the future

some examples would be good also to add, and a mention in the documentation wouldn't hurt

src/util.rs Outdated
@@ -51,6 +51,10 @@ pub struct Config {
pub feature_peripheral: bool,
#[cfg_attr(feature = "serde", serde(default))]
pub max_cluster_size: bool,
#[cfg_attr(feature = "serde", serde(default))]
pub impl_register_debug: bool,
Copy link
Member

Choose a reason for hiding this comment

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

I'd like it if this field took a enum instead, which would make it possible to use defmt instead in the future.

Ofcourse, this can be added later with a breaking change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that or add another bool option for impl_defmt allowing both to be specified.

src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@liebman
Copy link
Contributor Author

liebman commented May 30, 2023

@burrbull Here is a (truncated) sample run on an esp32s3 using println!("RTC_CNT {:#?}", unsafe { &*esp32s3::RTC_CNTL::ptr() });

RTC_CNT RegisterBlock {
    options0: OPTIONS0 {
        sw_stall_appcpu_c0: 0,
        sw_stall_procpu_c0: 0,
        bb_i2c_force_pd: false,
        bb_i2c_force_pu: false,
        bbpll_i2c_force_pd: false,
        bbpll_i2c_force_pu: false,
        bbpll_force_pd: false,
        bbpll_force_pu: false,
        xtl_force_pd: false,
        xtl_force_pu: false,
        xtl_en_wait: 2,
        xtl_force_iso: false,
        pll_force_iso: false,
        analog_force_iso: false,
        xtl_force_noiso: true,
        pll_force_noiso: true,
        analog_force_noiso: true,
        dg_wrap_force_rst: false,
        dg_wrap_force_norst: false,
    },
    slp_timer0: SLP_TIMER0 {
        slp_val_lo: 0,
    },
    slp_timer1: SLP_TIMER1 {
        slp_val_hi: 0,
    },
    time_update: TIME_UPDATE {
        timer_sys_stall: false,
        timer_xtl_off: false,
        timer_sys_rst: false,
    },
    time_low0: TIME_LOW0 {
        timer_value0_low: 46911,
    },
    time_high0: TIME_HIGH0 {
        timer_value0_high: 0,
    },
    state0: STATE0 {
        apb2rtc_bridge_sel: false,
        sdio_active_ind: false,
        slp_wakeup: true,
        slp_reject: false,
        sleep_en: false,
    },
<snip>

@liebman
Copy link
Contributor Author

liebman commented May 30, 2023

@Emilgardis I like the naming suggestions and will try to update examples and documentation.

@liebman liebman dismissed stale reviews from Emilgardis and burrbull via 49cf7ee May 30, 2023 17:34
src/generate/register.rs Outdated Show resolved Hide resolved
src/generate/register.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@liebman liebman requested a review from Emilgardis May 30, 2023 21:59
Copy link
Member

@Emilgardis Emilgardis left a comment

Choose a reason for hiding this comment

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

awesome! thank you :)

I think we can merge this, I'll leave the final r+ for @burrbull

@Emilgardis
Copy link
Member

Might be good to squash some of the commits btw!

@liebman
Copy link
Contributor Author

liebman commented Jun 1, 2023

squashed

@Emilgardis Emilgardis requested a review from burrbull June 2, 2023 07:07
@Emilgardis
Copy link
Member

bors r=emilgardis,burrbull

thank you!

@bors
Copy link
Contributor

bors bot commented Jun 2, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 8f63b56 into rust-embedded:master Jun 2, 2023
@liebman liebman deleted the impl-debug-for-registers branch June 2, 2023 12:58
@liebman
Copy link
Contributor Author

liebman commented Jun 2, 2023

Is there a date planned for the next release?

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.

3 participants