Add support RTT for 64-bit processors #2296

tnishinaga · 2024-03-16T17:32:26Z

I added support RTT for 64-bit processors.

The 64-bit mode is not yet supported(TBD) by SEGGER RTT.
https://github.com/SEGGERMicro/RTT/blob/master/RTT/SEGGER_RTT.h#L216-L217

But I created a code that works with the defmt_rtt.
https://github.com/knurling-rs/defmt/blob/main/firmware/defmt-rtt/src/lib.rs#L124-L136

I checked it works on Raspberry Pi 4(64bit) and Raspberry Pi Pico(32bit).

Yatekii

Thanks a lot for implementing this!

Is there a reason we want to allow more channels in 64bit archs?

tnishinaga · 2024-03-28T15:50:11Z

Is there a reason we want to allow more channels in 64bit archs?

I thought I didn't change the number of max channels.
Could you tell me which part you are pointing out?

bugadani · 2024-03-30T12:20:29Z

probe-rs/src/rtt.rs

        // Memory contents read in advance, starting from ptr
        mem_in: Option<&[u8]>,
    ) -> Result<Option<Rtt>, Error> {
+        let is_64bit = match core.instruction_set()? {


This would be a lot nicer as if matches!(core.instruction_set()?, probe_rs_target::InstructionSet::A64). We only need to list the ones that are 64-bit. I realize the downside is that adding new architectures are immediately treated as 32-bit, though, so maybe instead this logic could be moved into a function like InstructionSet::width()?

so maybe instead this logic could be moved into a function like InstructionSet::width()?

In this code, I want to know the size of the target integer(32bit or 64bit).
I'm checking it from the InstructioSet now, but I think there has to be a better than the current method.
Could you please tell me if you know of any good ideas?

I found that Armv8a state has an is_64_bit flag, so I changed this code to use it.
008aa3a

bugadani · 2024-03-30T12:21:51Z

probe-rs/src/rtt.rs

-                core.read(ptr.into(), &mut mem)?;
+                let mut mem = vec![
+                    0u8;
+                    if is_64bit {


Could you instead store the width-dependent constants in fields first, to avoid a bunch of (mostly) unnecessary if-else?

In other places, you could probably abstract away the if 64-bit { do64 } else { do32 } code into helper functions just to clean the implementation up a bit. I'm not sure what would be the nicest, but I'm sure we can do better than an if-else everywhere.

I'm sure we can do better than an if-else everywhere.

I agree.
I am refactoring RTT codes, but it will take time to complete.

tnishinaga@72758b0

So, I would be happy if you could merge the current code first.

Thank you for waiting.
The refactor is complete now 04f918e.
Could you please review it again?

probe-rs/src/rtt/channel.rs

Refactor RTT

bugadani

Looks better, thank you for working on this. I have a few observations that I think could improve this PR slightly, please take a look.

bugadani · 2024-04-08T08:28:15Z

probe-rs/src/rtt.rs

@@ -260,7 +411,7 @@ impl Rtt {
            .into_iter()
            .filter_map(|range| {
                let range_len = match range.end.checked_sub(range.start) {
-                    Some(v) if v < Self::MIN_SIZE as u64 => return None,
+                    Some(v) if v < (if is_64_bit {std::mem::size_of::<RttControlBlockHeaderInner<u64>>()} else {std::mem::size_of::<RttControlBlockHeaderInner<u32>>()} as u64) => return None,


This isn't the most elegant way,

Complex guard conditions like this are hard to understand.

This is the third place where you implement the bool -> pointer width conversion. You should probably add a RttControlBlockHeader(Inner?)::size(is_64_bit: bool) function.

Thank you for your review!
I fixed it 22e9c7c

bugadani · 2024-04-08T08:28:59Z

probe-rs/src/rtt.rs

@@ -250,8 +401,8 @@ impl Rtt {
                tracing::debug!("Scanning region: {:?}", region);

                vec![Range {
-                    start: region.start as u64,
-                    end: region.end as u64,
+                    start: region.start,


Can this be a region.clone()?

Thank you!
e8741b5

bugadani · 2024-04-08T08:31:10Z

probe-rs/src/rtt/channel.rs

+    }
+
+    pub fn flags_offset(&self) -> usize {
+        std::mem::size_of::<Self>() - std::mem::size_of::<T>()


Can we use offset_of! here? Also, considering these functions are (almost?) always used to calculate some memory access offset, I think it would be better if all three of these returned u64

bugadani · 2024-04-08T08:40:57Z

probe-rs/src/rtt/channel.rs

+    }
+
+    pub fn read_buffer_ptr_offset(&self) -> usize {
+        std::mem::size_of::<RttChannelBufferInnerHeader<T>>() + std::mem::size_of::<T>()


This is pretty hard to understand because the second size_of points in the middle of an inner struct. Although offset_of_nested is not stable yet in Rust, this might still be a similarly good place to use offset_of!, like std::mem::offset_of!(Self, buffer_offset) + std::mem::offset_of!(RttChannelBufferInnerBufferOffset<T>, read_offset)

Thank you for sharing your good method!

I rewrote *_offset functions using offset_of!.
0473385

Tiwalun · 2024-04-08T18:29:19Z

The 64-bit mode is not yet supported(TBD) by SEGGER RTT.
https://github.com/SEGGERMicro/RTT/blob/master/RTT/SEGGER_RTT.h#L216-L217

Just so you're aware, that repository seems to be outdated. The latest code which you can download from the Segger website does seem to support 64-bit mode.

tnishinaga · 2024-04-09T16:48:16Z

Just so you're aware, that repository seems to be outdated. The latest code which you can download from the Segger website does seem to support 64-bit mode.

Thank you for letting me know 😄

I downloaded the latest codes(SEGGER_RTT_V796c.tgz) from https://www.segger.com/downloads/jlink#J-LinkSoftwareAndDocumentationPack and I checked these codes.
However, I could not find any code that supports ARMv8a.

Could you please tell me where is the codes to support 64-bit mode?

Tiwalun · 2024-04-10T18:17:02Z

Could you please tell me where is the codes to support 64-bit mode?

You might be right, I saw that they support Cortex-A and assumed that meant 64 bit. But it seems they only support ARMv7-A, which is 32-bit.

Of course, the Segger C code just uses unsigned, so I guess it changes layout automatically...

bugadani · 2024-05-16T21:16:59Z

Thank you very much :)

tnishinaga and others added 3 commits March 16, 2024 15:45

Add support RTT for 64bit processor

71b0d59

Merge branch 'master' into support_64bit_rtt

5a03322

fixup! Add support RTT for 64bit processor

a76a44e

tnishinaga changed the title ~~Add support RTT for 64bit processors~~ Add support RTT for 64-bit processors Mar 17, 2024

tnishinaga and others added 4 commits March 28, 2024 00:18

Merge branch 'master' into support_64bit_rtt

93a7395

fixup! Merge branch 'master' into support_64bit_rtt

29b7ba0

fixup! fixup! Merge branch 'master' into support_64bit_rtt

2e425b5

Merge branch 'master' into support_64bit_rtt

103d2b5

Yatekii reviewed Mar 28, 2024

View reviewed changes

bugadani reviewed Mar 30, 2024

View reviewed changes

tnishinaga and others added 9 commits April 1, 2024 02:14

remove comment

fc89d0b

RTT: RTT code refactoring

04f918e

Add is_64_bit function to select 32-bit or 64-bit RTT header

008aa3a

Merge branch 'support_64bit_rtt' into refactor_rtt

6a13b22

Merge pull request #1 from tnishinaga/refactor_rtt

a9f7d87

Refactor RTT

Merge branch 'master' into refactor_rtt

a50b767

fixup! Merge branch 'master' into refactor_rtt

dfb2375

Merge branch 'refactor_rtt' into support_64bit_rtt

a6e6d73

fixup! Merge branch 'refactor_rtt' into support_64bit_rtt

9289d7e

bugadani requested changes Apr 8, 2024

View reviewed changes

tnishinaga added 5 commits April 9, 2024 01:59

RTT: region.clone()

e8741b5

RTT: add minimal_header_size function to simplify the code

22e9c7c

fixup! RTT: add minimal_header_size function to simplify the code

1478073

RTT: use offset_of to simplify the code

0473385

RTT: more simplify

c6a5106

Merge branch 'master' into support_64bit_rtt

1730465

Merge branch 'master' into support_64bit_rtt

3bbea51

bugadani requested a review from Yatekii May 11, 2024 14:02

bugadani approved these changes May 16, 2024

View reviewed changes

bugadani added this pull request to the merge queue May 16, 2024

Merged via the queue into probe-rs:master with commit bdf2f46 May 16, 2024
19 checks passed

tnishinaga deleted the support_64bit_rtt branch May 17, 2024 02:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support RTT for 64-bit processors #2296

Add support RTT for 64-bit processors #2296

tnishinaga commented Mar 16, 2024

Yatekii left a comment

tnishinaga commented Mar 28, 2024

bugadani Mar 30, 2024 •

edited

tnishinaga Mar 31, 2024

tnishinaga Apr 6, 2024

bugadani Mar 30, 2024 •

edited

tnishinaga Mar 31, 2024

tnishinaga Apr 6, 2024 •

edited

bugadani left a comment

bugadani Apr 8, 2024

tnishinaga Apr 8, 2024

bugadani Apr 8, 2024

tnishinaga Apr 8, 2024

bugadani Apr 8, 2024

bugadani Apr 8, 2024

tnishinaga Apr 8, 2024

Tiwalun commented Apr 8, 2024

tnishinaga commented Apr 9, 2024

Tiwalun commented Apr 10, 2024

bugadani commented May 16, 2024

Add support RTT for 64-bit processors #2296

Add support RTT for 64-bit processors #2296

Conversation

tnishinaga commented Mar 16, 2024

Yatekii left a comment

Choose a reason for hiding this comment

tnishinaga commented Mar 28, 2024

bugadani Mar 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bugadani Mar 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnishinaga Apr 6, 2024 • edited

Choose a reason for hiding this comment

bugadani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tiwalun commented Apr 8, 2024

tnishinaga commented Apr 9, 2024

Tiwalun commented Apr 10, 2024

bugadani commented May 16, 2024

bugadani Mar 30, 2024 •

edited

bugadani Mar 30, 2024 •

edited

tnishinaga Apr 6, 2024 •

edited