-
Notifications
You must be signed in to change notification settings - Fork 7
Fix detection of some registers #6
Comments
I haven't had a chance to dig into this too deep yet, but I think I may have found the source of the problem. For certain definitions, the transition from I printed out a bit of debug info to show this. I've included a couple of examples below:
As you can see in both these cases, the previous line is the one we want to be matching on. The line number printed in the error message corresponds with the previous line. |
Thanks for looking into this! I've just poked around in those files, and in those two instances, I think the parser it behaving normally. The way idf2svd works is by finding defines that end in The full snippet from uart_reg.h looks like this #define UART_FIFO_AHB_REG(i) (REG_UART_AHB_BASE(i) + 0x0) // idf2svd expects a comment with bitfield info below here
#define UART_FIFO_REG(i) (REG_UART_BASE(i) + 0x0) // and below here
/* UART_RXFIFO_RD_BYTE : RO ;bitpos:[7:0] ;default: 8'b0 ; */
/*description: This register stores one byte data read by rx fifo.*/
#define UART_RXFIFO_RD_BYTE 0x000000FF
#define UART_RXFIFO_RD_BYTE_M ((UART_RXFIFO_RD_BYTE_V)<<(UART_RXFIFO_RD_BYTE_S))
#define UART_RXFIFO_RD_BYTE_V 0xFF
#define UART_RXFIFO_RD_BYTE_S 0 I think this more of an issue with the consistency of the documentation of the idf; though with that in mind, what is the best course of action to take. I only looked at uart and uhci so far but its possible that there are bugs in the parsing for other peripherals. |
From page 351 in the reference manual Ideally the parser should really be able to cope with this but as far as I can tell |
In RTC_CNTL, the read only info uses a '0' instead of a 'O'. /* RTC_CNTL_RDY_FOR_WAKEUP : R/0; bitpos:[19]; default: 0 */ should be /* RTC_CNTL_RDY_FOR_WAKEUP : R/O; bitpos:[19]; default: 0 */ |
Gotcha, thanks for clearing that up. The output just seemed off to me initially. The error
(Note the addition of |
Ah good spot! Would you mind PR'ing that change to the regex? For the items where the docs are wrong (or missing), I guess we'll have to fix the docs in our own fork (unless you have any other ideas?) but I think its better to worry about that later. |
I've submitted a PR to fix the register parsing issue. The items outlined in the first comment still need to be resolved at this point. I'm not against maintaining our own fork, as that's probably easier and more reliable than trying to be clever and coding around the inconsistencies. But as you said that can probably be addressed at another time, it's documented here at least for the time being. |
Hi there, I recently commented on your reddit thread about helping out. Still reading through the code, but here are my assumptions so far, please correct me if I'm wrong:
Just trying to figure out the end state here. Also, it seems even if not every register got matched, the SVD file is still usable for the registers that did get matched, right? |
Essentially, yes. atsamd-rs has some SVDs from Microchip if you'd like to see some complete examples.
Correct on both. A good chunk of the registers failing to parse seem to be due to inconsistencies in the esp-idf source as far as I can tell.
As done as anything ever is 😉 esp-idf will continue to be updated I'd imagine so there will likely be tweaks required here and there, but in theory once all peripherals are documented by the SVD file then we have nothing left to do.
That seems like a reasonable simplification, though we'd like to get information on interrupts as well (and I'm sure there are further details we can extract).
The registers which are being matched have PAC code generated for them currently; this is handled by the esp32 repo. In theory if the register is being parsed you should be able to manipulate it programmatically. Hopefully that helps, anything else I'll leave to @MabezDev |
(ESP-IDF maintainer here) We have an issue filed for providing SVD files: espressif/esp-idf#2214. Probably we will solve this for the upcoming ESP32-S2 chip first, because we have started using an internal register description format which can be converted to SVD (some code needs to be written...). For the ESP32, this may take more time because we didn't have that format in place when ESP32 was developed. I know this doesn't solve the present day issue, just wanted to say that we are aware of this need and sorry for making you jump through hoops to generate SVDs. |
@jessebraham Thanks for the details. Originally I thought maybe using a C preprocessor would be easier, but probably a hand-rolled solution is easy enough so I'll try to work through the code here. @igrr That's great to hear espressif will have official SVDs at some point! |
@jessebraham has summed it up quite nicely. It's a two step process,
No problem! It was actually pretty fun getting it all working, but I'd be much nicer to have a verified SVD from yourselves. Hopefully generating a esp32-s2 PAC will be a lot less work :) |
So given the unknown timeline, I suppose we'll continue with this solution until espressif puts out an official SVD? I'm still a little unfamiliar with what we want to do when we run into particular registers. As an example, I'm on the current master, and this is the first register matching error that occurs:
And the contents of those lines: 150 #define RTC_I2C_SLAVE_ADDR_S 0
151
152 /* Result of last read operation. Not used directly as the data will be
153 * returned to the ULP. Listed for debugging purposes.
154 */
155 #define RTC_I2C_DATA_REG (DR_REG_RTC_I2C_BASE + 0x01c)
156
157 /* Interrupt registers; since the interrupt from RTC_I2C is not connected,
158 * these registers are only listed for debugging purposes.
159 */
160
161 /* Interrupt raw status register */
162 #define RTC_I2C_INT_RAW_REG (DR_REG_RTC_I2C_BASE + 0x020) So that register is only listed for debugging purposes - do we still want to document or is it okay to leave it absent? A lot of the other failed registers are ones that are indexed, what do we want to do with those? Sorry for all the questions, I think I understand the overall goal but I'm not sure what actions we want to take when we find these unusual register definitions. |
RTC_I2C is mostly useable from the ULP coprocessor of the ESP32, so it shouldn't matter for writing Rust programs running on Xtensa cores. I would suggest skipping this file entirely.
I can keep an eye on this topic and answer your questions, if it helps. |
Yeah seeing as the esp32-s2 isn't even out yet I suspect it will be quite a while for the esp32 svd to show up.
Me too to be honest; earlier on in the thread I suggested forking the idf and fixing the doc comments, which we we eventually fold back into upstream idf. Perhaps it might be better to just create the peripherals manually in
I think that would be very helpful. |
If it makes sense, I'll go ahead and run on the latest master and put together a table of all the registers that failed to match, and link to their source in the esp-idf. |
Kinda ugly but maybe this is useful? Ran on idf2svd @ commit f370ddb
|
frc_timer_reg.h: this file was written by hand, not generated by a tool, hence the different format. I think it is unlikely that you will need to use this timer in Rust code, since there are 4 64-bit timers in the Timer Group peripheral which are much easier to use. We do use this timer in IDF for the spi_reg.h:135: looks like some manual fixup was made. This register should have a 32-bit r/w field inside. i2c_reg.h:944: this is the address of a 32-byte hardware FIFO uart_reg.h:22: For UART FIFO, we use different base address than for the rest of the register. On the ESP32, the peripherals are mapped into two address ranges, 0x3ffxxxxxx and 0x600xxxxxx. The former allows for faster access, but the latter guarantees that the CPU will not issue speculative reads. This distinction is important when accessing the FIFOs. hwcrypto_reg.h: Also not generated by a tool. All registers (ending with _REG) are 32-bit, and there are some register banks, their names end with _BASE. rmt_reg.h: These data registers are 32-bit registers, contain a single r/w field. UHCI_ACK_NUM_REG: Not entirely sure what this one does, we haven't implemented support for UHCI peripheral in IDF. (This is basically a UART DMA engine.) sdmmc_reg.h: Not generated by a tool, so there is no breakdown of registers into fields for this peripheral. There is a sdmmc_struct.h file, which does contain field definitions. If I recall correctly, it was written by hand, as we didn't have a machine-readable description of the SDMMC registers. rtc_cntl_reg.h#L1833: not sure why this was flagged, definition looks okay to me. HOST_SLCHOST_WIN_CMD_REG: a R/W register, has a single 32-bit field. |
@igrr Thanks so much! I'll update the table in a bit. Regarding |
@MabezDev I updated the table in my comment above, hopefully it helps. It feels like we could go forward with just manually creating the missing registers, there aren't a huge amount of them. |
@bschwind Thank you for taking the time to format this nicely! I would tend to agree, I think we've got 99~>% coverage so I think it would be best to focus on other aspects in esp32. I've been pretty busy these last few days, but I will eventually put up some issues on the esp32 repo. I'll be sure to ping this thread when they are up. |
So I've started creating some issues: idf2svd:
xtensa-lx6-rt:
esp32: |
The current output from master shows the failed parsing of some registers.
The text was updated successfully, but these errors were encountered: