Skip to content
This repository has been archived by the owner on Dec 15, 2021. It is now read-only.

RFC: replace crate with itm-decode #41

Closed
wants to merge 140 commits into from

Conversation

tmplt
Copy link

@tmplt tmplt commented Nov 25, 2021

As discussed in the rust-embedded matrix channel this PR replaces the implementation of itm with that of a refactored itm-decode. This new implementation offers a similar std::io::Read-based API, but with:

  • more granular error enums (MalformedPacket);
  • re-alignment of the trace stream by handling the synchronization ITM packet; and
  • chrono::DateTime-timestamped trace packets by intercepting global and local timestamp packets.

The new API is based on Iterator: Decoder<R: Read>::singles returns an iterator that yields single TracePackets; Decoder<R: Read>::timestamps returns an iterator that yields TimestampedTracePackets.

Planned for the future is an additional iterator that intercepts and handles the arbitration (see #39) of TracePacket::Instrumentations and TracePacket::Extensions which would yield the stings written on the target via cortex_m::{iprint, iprintln}. A timestamp should be associated to the strings also.

Some work still remains to be done before I consider this PR ready for merge (presuming the team vote mentioned in the matrix channel discussion passes), namely documentation and more tests for Decoder<R: Read>::timestamps.

Below I'll review the current state of the reimplementation and request comments on certain parts.

Presuming this PR is merged: a v0.4.0 of itm will be tagged and released; issues from itm-decode will be moved here; and itm-decode will be deprecated in favour of itm 0.4.0 and above.


Closes #36

CC @adamgreig

tmplt added 30 commits February 19, 2021 15:21
Changes:
- All log entry calls have been commented out. They are kept for
  documentation purposes when extending the implementation, but will
  ultimately be removed; this library will not operate with logs.
- Enums no longes derive Serialize/Deserialize. It was not immediately
  obvious which crate was responsible for this feature.
The stm32f401re seems only emit these sync packets. This implementation
suffices for target-dependent debugging.
LocalTimestamp2 contain all needed information in the header. No need to
be refactored.
This will be useful if any manual intervention must be done.
Upon an error like this, it may be necessary to manually decode the
data.
There is no subsequent processing that need be done after an overflow.
When I started writing this library I extracted the internal decoder
from probe-rs with the intention of refactoring it. Instead I ended up
rewriting the decoder from scratch. The current code base contains only
original code (when compared to the probe-rs implementation).
From experiments with an stm32f401re, the ARM specification may not be
followed in practise. Whe the decoder returns an error, it may thus be
necessary to manually (outside of the crate) inspect the incoming bytes
and eventually modify them to handle any possible target-specific
bitstreams which diverge from the spec. So instead of figuring out a
decent API for this intervention, we'll just make the internal state of
the decoder public.
Input is a bitstream which may eventually break the original 8-bit
alignment. Using a [u8] would then mean bookkeeping how much we diverge
from this alignment and appropriately process incoming bits, which adds
needless compexity for such an edge case. Using a BitVec keeps code easy
to read.
@tmplt
Copy link
Author

tmplt commented Nov 27, 2021

I'm happy with the state of the test bench for now. More should be added that tests GTS flags and additional compression, but the target I'm working with does support generation of global timestamps; I cannot dedicate more time than I already have on GTS functionality/testing.

While I'd prefer the API to be async I'm happy with the current implementation for a v0.4.0 release. An async API can be released with v0.5.0.

Next few commits I'll document the crate, after which I consider the PR ready for merge/voting.

@tmplt
Copy link
Author

tmplt commented Nov 27, 2021

Current implementation returns an absolute timestamp which requires the user to supply when the target was reset. As far as I understand this cannot be done nanosecond-accurately. Global and local timestamps are however exact and relative from target reset/boot. Should the chrono::DateTime (the accuracy of which depends on user-supplied data) be replaced by a chono::Duration (the accuracy of which will always be nanosecond-exact, disregarding the rounding done in calc_offset)?

If one would want an absolute timestamp, one need only to std::ops::Add the returned chrono::Duration to a user-managed chrono::DateTime.

@Emilgardis
Copy link
Member

is there a reason to stick with chrono instead of the time crate?

@tmplt
Copy link
Author

tmplt commented Nov 28, 2021

Not necessarily. I was not aware of time.

tmplt added 2 commits December 3, 2021 11:09
This commit can later be reverted if the crate is readopted into the WG.
This commit can later be reverted if the crate is readopted into the WG.
@oceanlewis oceanlewis mentioned this pull request Dec 10, 2021
12 tasks
@tmplt
Copy link
Author

tmplt commented Dec 14, 2021

rust-embedded/wg#589 has merged. This PR is no longer relevant. Closing.


@davidarmstronglewis, the library has been re-implemented and a v0.4.0 will be published soon. This fork contains itm (the library) and itm-decode (the CLI tool).

@tmplt tmplt closed this Dec 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cortex-m T-tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crate reimplemented with more features. Replace?
5 participants