Initial Surfer support #5
Conversation
| }, | ||
| { | ||
| "command": "rtlDebugger.addToWaveform", | ||
| "when": "view == rtlDebugger.sidebar && viewItem =~ /variable/", |
There was a problem hiding this comment.
This is wrong, it should check if the waveform viewer is active but I can't figure out how
|
The amount of whitespace and unrelated changes makes this quite difficult to review. |
|
Edit: No, diffing against |
|
I'm looking at the PR diff. |
|
Alright, cleaned up some of it |
| } | ||
| export class ServerPacketString { | ||
| constructor(public inner: string) { } | ||
| } |
There was a problem hiding this comment.
These classes don't seem like they're doing anything useful?
There was a problem hiding this comment.
I added them because I wanted type safety when passing around different kinds of strings
src/ui/waveform.ts
Outdated
| @@ -45,8 +74,26 @@ export class WaveformProvider { | |||
| console.log('[RTL Debugger] [WaveformProvider] Ready'); | |||
| } else if (message.type === 'crash') { | |||
| console.log('[RTL Debugger] [WaveformProvider] Crash:', message.error); | |||
| } else if (message.type === 'cxxrtl_csmessage') { | |||
There was a problem hiding this comment.
| } else if (message.type === 'cxxrtl_csmessage') { | |
| } else if (message.type === 'cxxrtl_cs_message') { |
| context.subscriptions.push(vscode.commands.registerCommand('rtlDebugger.addToWaveform', (treeItem) => { | ||
| if (rtlDebugger.waveformProvider) { | ||
| rtlDebugger.waveformProvider.addVariable(treeItem.designation.variable); | ||
| } |
There was a problem hiding this comment.
This seems like it would allow for exactly one Surfer tab to exist.
There was a problem hiding this comment.
Yep, so far I've been working under the assumption that this would be the case
There was a problem hiding this comment.
The existing code doesn't enforce it (intentionally--I think that would be fairly limiting if it was the only thing possible...)
There was a problem hiding this comment.
Yeah, makes sense. It does open a bunch of UI questions about where interactions should happen, for example which surfer should add a variable when you click +
There was a problem hiding this comment.
I think the VS Code UI maintains an "active tab" or "selected tab" or something like that. Barring that, simply the last one that received focus will match user expectations.
There was a problem hiding this comment.
Pushed an update to allow multiple viewers. I'm not very happy with the code since it requires maintaining my own list of tab->webview mappings that are based on the weird ViewType string but as far as I can tell, there is no other convenient way to do it...
This adds initial support for viewing waveforms from the debugger in Surfer. For now, it is only possible to open Surfer via the command prompt (
Browse waveforms). Most of the changes are on the Surfer side, not here, the changes here are primarily to embed surfer, and forward WCP and CXXRTL messages back and forth. There are some UI changes, in particular, a button to add signals to the waveform view.This should be seen as a proof of concept rather than a fully featured implementation. For now, the CXXRTL implementation on the Surfer side is mature and pretty well tested. The interaction between Surfer and the CXXRTL server via the extension also seems to work flawlessly.
There is less interaction between Surfer and the extension than I'd like. For example, there is no rendering of assertions (though that would be easy to add), and while we discussed having the Surfer time cursor synchronized between Surfer and the extension to show "correct" values in the VSC sidebar, that is something that I have not had time to implement (or know how to do on the extension side). That said, the WCP that we use for bidirectional control of and from the Surfer interface appears to work quite well, and does support the use case.
There are also some small UI papercuts remaining. The user can still use surfer keybinds to (accidentally(?)) open surfer sidebar and menus which in the extension are intended to be the VSCode native sidebar of the debugger. There is also a non-native sidebar for signal values in surfer, but removing and replacing it with a native sidebar would be quite a lot of work.
There is also no synchronization of signal value translation between surfer and the extension. For example, if you set the radix of a signal in the extension, that choice will not be reflected in Surfer and vice versa. Surfer also has a much more capable translation system which means that just synchronizing them isn't an option.
In addition to these papercuts, there are a few big picture things that probably need resolution before this can be more than a proof of concept:
Memories are unsupported
This is largely a Surfer side issue as it hasn't had a way to group related signals together. At the moment, adding a memory causes Surfer to crash since it sends an invalid request to the CXXRTL debugger. This is something I can address, but supporting memories requires a bit more work. There are some relevant Surfer issues here https://gitlab.com/surfer-project/surfer/-/issues/353 and https://gitlab.com/surfer-project/surfer/-/issues/97. There is also a UX/performance issue when adding very large memories.
The CXXRTL protocol is relatively slow, so sending full memory content when the user is only interested in some cells is not a particularly good idea. Being able to add individual memory cells might work for now.
Aliasing
In order to maintain speed, Surfer renders signals by binary searching for the signal value at each pixel. This results in
O(log(n))lookup which is needed for reasonable performance on long simulation traces. However, this causes aliasing (in the signal theory sense, not the graphics sense)For example: at one zoom level a clock signal looks like this
but zoomed in one more step, it goes back to its correct value
In file based formats, this is solved by the fact that we have a list of time steps at which a signal changed, so we can check if it changed between two adjacent pixels, even if the value is the same at both pixels.
However, with the cxxrtl protocol, we get the timestamps at which any signal changed, which means that we can't figure out if there is a transition between two pixels without a linear search across all the time steps.
There may be some solutions to this that we could apply, for example we could walk the signal values once when we receive them and build a new structure, but that would require initial work when signals change.
As far as I understand, the server is in theory able to do this anti-aliasing (mipmaping) much more efficiently, and it could be integrated into the protocol as an additional parameter to the
query_intervalcommand.Performance
In the current implementation, Surfer fetches signals for the whole simulation time range. This is needed because we want to show something when the user is zoomed out, and Surfer can already efficiently render signals with many time steps by binary searching for the correct value (kind of, see the aliasing note above). However, the CXXRTL protocol is quite slow, so fetching signals for long simulation (tens of thousands of samples) makes things noticeably slow, while even longer simulations make the program appear frozen. Again, this is something I cannot do anything about from the surfer side. Mipmaping on the server would solve the issue as it'd only send values for specific time steps that Surfer asks for.
Another option might be to strategically
query_intervalonly for the values at which Surfer intends to render signals. However, this would result in a lot of traffic (one JSON request per pixel), and would require quite significant changes to the way queries are handled.As a workaround which @robtaylor asked for is to only render the last N samples. The problem with that is that it is not possible for Surfer to know the sample rate (and it can only request time ranges), and as I understand it, the sample rate is not well defined on the server side (not well known for well behaved single clock domain projects, undefined for others). For now, I have made a change to Surfer that assumes a 1us sample rate, and requests 10k samples. This should work as a POC as long as users ensure that they obey that sample rate. If however, they use another sample rate like 1_ns as is done in the example in this repo, the traces will still be too long and it will appear as if the program is frozen for long running simulations.