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 itm tracing #145

Merged
merged 42 commits into from Jul 21, 2020
Merged

Implement itm tracing #145

merged 42 commits into from Jul 21, 2020

Conversation

Yatekii
Copy link
Member

@Yatekii Yatekii commented Feb 16, 2020

No description provided.

…public interface better structured. Also document stuff.
…ed fresh from reset it has an invalid preamble in the romtables; when @widelbouwman's swviewer is connected once it reads properly afterwards. so it seems like we forget to enable something
…ich ties off all debug components to save energy; It is questionable why this should give bogus reads of the romtables
Ensure compilation.
@adamgreig
Copy link
Member

Nice! This worked right away for me with an st-link on an stm32f4 nucleo board, although it kicked up some errors about reading the ROM tables. I don't know if there's some stm32-specific thing to do for that? These errors didn't seem to cause any actual problems.

 ERROR probe_rs::architecture::arm::memory::romtable > Writing reg: e0040004 = 1
 ERROR probe_rs::architecture::arm::memory::romtable > Writing reg: e0040010 = 7
 ERROR probe_rs::architecture::arm::memory::romtable > Writing reg: e00400f0 = 2
 ERROR probe_rs::architecture::arm::memory::romtable > Writing reg: e0040304 = 100
 ERROR probe_rs::architecture::arm::memory::romtable > Writing reg: e0000fb0 = c5acce55
 ERROR probe_rs::architecture::arm::memory::romtable > Read reg: e0000e80 = d000f
 ERROR probe_rs::architecture::arm::memory::romtable > Writing reg: e0000e80 = d000f
 ERROR probe_rs::architecture::arm::memory::romtable > Writing reg: e0000e00 = ffffffff
 ERROR probe_rs::architecture::arm::memory::romtable > Read reg: e0001000 = 40000401
 ERROR probe_rs::architecture::arm::memory::romtable > Writing reg: e0001000 = 40000401

Forwarding the SWV access through the session object seems sensible to me and the example seems to work nicely with it. I think that user-level API (session.trace_enable(), session.read_swv, decoder.feed(bytes)) all seems fine. Is there a reason to say trace in some method names but swv in others?

The SwvReader trait for probes is simple to implement, but possibly we should allow read() to take a timeout or be blocking so it doesn't have to poll the probe for how much data is available before trying to read the USB endpoint. More thoughts about that in next paragraph. Also, right now all probes would have to enable SWO readout at attach even if it's not used, but this could add overhead on some probes. On many targets SWO might not even be connected so could just see random noise which would needlessly waste USB bandwidth. Perhaps we could have an enable() method in SwvReader which is only called if SWV is actually being used?

In the ST-link implementation you first request how many bytes are available, and then try to read exactly this many bytes, erroring if you get less. That way if there's no bytes available you can immediately return an empty Vec instead of waiting for data. However at higher data rates this means an extra USB round trip for every transfer which will limit throughput. If we could allow SwvReader::read() to block or specify a timeout, you could instead just read whatever is available in the USB endpoint and return that. For clients that run SWV in a separate thread or are only interested in SWV, maybe this would work better and allow higher throughput?

I'm interested in adding DAPlink support. There are two ways DAPlink devices can send SWO data, either polled (host sends "send me at most N bytes of SWO data", device replies "here are M bytes of data" where 0<=M<=N) over either the HID or v2 endpoints, or if the probe supports it, the probe simply streams all SWO data to a dedicated bulk endpoint, which looks to be the same as the ST-link. There is also a command for "how much data do you have?" without transferring the data. I think this should be pretty easy to add to this design.

Have you thought about how SWV might work alongside the gdb server? I think it's a similar problem to RTT in that we want two things to access the device at the same time so they might conflict, but with SWV it could be easier, since we're not touching device registers, just reading a data stream. Maybe this can be something for later though.

@Yatekii
Copy link
Member Author

Yatekii commented Apr 26, 2020

Nice! This worked right away for me with an st-link on an stm32f4 nucleo board, although it kicked up some errors about reading the ROM tables. I don't know if there's some stm32-specific thing to do for that? These errors didn't seem to cause any actual problems.

Awesome! Well, I am sorry for the errors. It was just a way for me to get the log messages up in the hierarchy and isolate them because the debug & trace logs are kinda cluttered ... should have removed them, sorry.

Is there a reason to say trace in some method names but swv in others?

No, I think I started naming things SWV halfways through the coding ... I am not sure what we should use best. But trace can be ambivalent.

Perhaps we could have an enable() method in SwvReader which is only called if SWV is actually being used?

Sound like a great idea to me!

If we could allow SwvReader::read() to block or specify a timeout, you could instead just read whatever is available in the USB endpoint and return that. For clients that run SWV in a separate thread or are only interested in SWV, maybe this would work better and allow higher throughput?

To be honest, I am always against blocking for more sophisticated applications. It doesn't allow for cancellation or similar things. Even when you run on a separate thread. A timeout could work :)
I see why you worry and I am sure we can find a good solution here :)

I think this should be pretty easy to add to this design.

Sounds great!

Have you thought about how SWV might work alongside the gdb server?

Yes, actually, I have put a lot of though into that.
I am just so very unsure what the best way to solve this is.

At the moment the Core struct contains an Rc<RefCell<dyn CoreInterface>> which means it can never be shared accross threads.
The pre 0.5.0 version of the API required you to pass the probe to every method of the Core struct/trait, which was very cumbersome.

Now the Core struct owns the Probe to not always have to pass the reference and also grant it unique access. Which means that the Session object does not have access to the Probe anymore. I think we could remove the Rc<RefCell<T>> tbh I am not even sure anymore why it is there. But This is still not so nice, because that way you'd only have one copy of the core which owns the Probe/Session which then cannot be accessed by others.

One thing we could do would be using an Arc<Mutex<T>> internally, but I am not sure if this is good design. Another could be that the Session can be used to get a new Core just like at the moment, but that the Core holds an &mut Probe which will be released once the Core goes out of scope. That would mean you still cannot have multiple consumers, but you could put the Session object into an Arc<Mutex<T>> and always get new Core handles once you locked the Session as a consumer.

Not sure my words/thoughts make any sense ... hopefully they do ...
What do you think about this?
We basically have been postponing this imo difficult descision & work forever now ...

@adamgreig
Copy link
Member

It was just a way for me to get the log messages up in the hierarchy and isolate them because the debug & trace logs are kinda cluttered ... should have removed them, sorry.

Ah, I thought they meant something was not working with the STM32, I've heard there are some oddities around reading its ROM tables but I don't really know anything about it.

No, I think I started naming things SWV halfways through the coding ... I am not sure what we should use best. But trace can be ambivalent.

I'm not really sure either, there are so many options.. I guess trace could also be used for the high-speed parallel tracing one day.

To be honest, I am always against blocking for more sophisticated applications. It doesn't allow for cancellation or similar things. Even when you run on a separate thread. A timeout could work :)
I see why you worry and I am sure we can find a good solution here :)

Perhaps: both STlink and the cmsis-dap endpoint-based implementations could call read_bulk with a timeout of 0/nearly 0, so they will immediately return anything waiting in the probe but not otherwise block.

Yes, actually, I have put a lot of though into that.

I will read and think about this part tomorrow!

@Yatekii Yatekii mentioned this pull request May 31, 2020
Ensure everything compiles correctly
@Yatekii
Copy link
Member Author

Yatekii commented Jun 20, 2020

Todo:

  • Use SWV instead of ITM everywhere where suitable.
  • Fix address tracing setup (works if only one address is traced. For 2 or more addresses it seems broken).
  • Make sure the tracing setup API is nice to use.
  • Make clock configurable.
  • Add enable() method to SwvReader.
  • Work out what to do about blocking in read().

Copy link
Member

@adamgreig adamgreig left a comment

Choose a reason for hiding this comment

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

Here we go!

@Yatekii
Copy link
Member Author

Yatekii commented Jul 21, 2020

Here goes nothing!

@Yatekii Yatekii merged commit 043b1b4 into master Jul 21, 2020
@Yatekii Yatekii deleted the itm branch July 21, 2020 13:16
Yatekii pushed a commit that referenced this pull request Jan 13, 2023
145: Update dependabot.yml r=Yatekii a=Tiwalun

Don't create dependabot PRs for all probe-rs updates.

Co-authored-by: Dominik Boehi <dominik.boehi@gmail.com>
burrbull pushed a commit to burrbull/probe-rs that referenced this pull request Feb 27, 2023
145: README.md: add troubleshooting section r=jonas-schievink a=Lotterleben

prompted by knurling-rs/probe-run#144 : add a troubleshooting section that explains what to look for on your board in order to be able to use `probe-run`.

while we're at it, also adds a section about udev rules as a band-aid for probe-rs#357 .

Co-authored-by: Lotte Steenbrink <lotte.steenbrink@ferrous-systems.com>
Co-authored-by: Jonas Schievink <jonas.schievink@ferrous-systems.com>
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.

None yet

2 participants