eclipse-embed-cdt / eclipse-plugins

The Eclipse Embedded CDT plug-ins for Arm & RISC-V C/C++ developers (formerly known as the GNU MCU Eclipse plug-ins). Includes the archive of previous plug-ins versions, as Releases.
http://eclipse-embed-cdt.github.io/
Eclipse Public License 2.0
554 stars 130 forks source link

Values are not listed on Peripherals Memory Window #512

Closed vishnusarathashling closed 2 years ago

vishnusarathashling commented 2 years ago

When we select some of the peripherals on Peripherals window. Those are listing on the Memory Window. but values are not updating. It is updating after some Step-in Operation.

vishnusarathashling commented 2 years ago

Please see the observations below:

Inside the org.eclipse.embedcdt.debug.gdbjtag.ui.MemoryBlockMonitor::addMemoryBlock function class we are creating an extended memory block, which is returning an object of PeripheralMemoryBlockExtension class. But in the constructor of PeripheralMemoryBlockExtension a new job (UpdatePeripheralRenderingJob) is created and scheduled (which runs in a different thread) which add the values to the Peripheral nodes. Parallely the UI thread will populate UI elements. Since the node values are populated in a different thread sometimes the values are not populated correctly. But when I updated the code so as to make the UI thread to wait until the UpdatePeripheralRenderingJob to get completed, the UI is showing proper values.

vishnusarathashling commented 2 years ago

I tried debugging this again, please see the observations below: We have the main UI thread (U) which creates a rendering job (run in a different thread T). This rendering job is actually setting the values to the PeripheralRegisterVMNode nodes. So once a peripheral register is selected.

u1. Thread U will schedule the job for thread T. u2. Then U will be creating the controls for PeripheralRendering (ie: in Memory view). u3. After creating the controls thread U will set the current values. u4. Then it will add the class PeripheralRendering to the Debug event Listeners.

(Once a DebugEvent is obtained the PeripheralRendering class will refresh the Memory UI correponding to a Peripheral register. This is what is happening during a suspend event.) During the completion process of the rendering job thread T will fire a DebugEvent which we expect the PeripheralRendering class to handle. But since U and T are different threads there is no guarantee that T is firing the Debug event after step u4 (mentioned above). If T fires the debug event before u4 PeripheralRendering class will not get the debug event and so it will not refresh value changes made by T. This is actually what is happening in this case. When I added several logs this is the conclusion I got. If T completes setting the node values before step u3 then also it will have the proper values. If T is setting the values to the node before u3, in this case also it should work fine. But this also we can't guarantee since both are different threads. So ideally we need to ensure that PeripheralRendering class should be handling any missed DebugEvent (already we have filtering based on Memory block source). Any missed DebugEvent between u2 and u4 should be handled (which will just refresh the UI with new PeripheralRegister node values) after all the controls are initialised. This could be done by adding the PeripheralRendering class to the Debug event Listeners initially itself, then cache the events received before creating the controls and process these events after the control are created. Already the PeripheralRendering class is handling the DebugEvents whose source is the memory block corresponding to this class.

Just scheduling the fRefreshJob at the end of PeripheralRendering::createControl method fixed the issue. @ilg-ul Please add your suggestions.

ilg-ul commented 2 years ago

Sorry for not having an answer for you, there are many years since that code was written.

But if you have a patch, I can consider it.

vishnusarathashling commented 2 years ago

@ilg-ul Please see the attached patch file. 0001-Refresh-the-UI-since-while-initialising-the-controls.txt .

jonahgraham commented 2 years ago

@vishnusarathashling can you provide it as a PR please so that it can be reviewed using the tooling on github?

ilg-ul commented 2 years ago

@jonahgraham, the patch is simple, basically a call to fRefreshJob.schedule();.

The question is how can we validate this?

jonahgraham commented 2 years ago

I see it as a no-harm patch. An extra refresh won't hurt anything, and the only side-effect of the refresh is changing the fading level, but as this is when the view is first created, there should be no harm in that. If it fixes the observable use case for @vishnusarathashling then it is fine, with a worse case that it doesn't fix the case.

ilg-ul commented 2 years ago

Ok, in this case @vishnusarathashling please submit a proper pull request with the change (on the develop branch), with the comment line but without the <CUSTOMISATION> inserts.

vishnusarathashling commented 2 years ago

@ilg-ul @jonahgraham I think I am not having access to create a branch, hence I am not able to create the PR. Do we have any other options ?

jonahgraham commented 2 years ago

Hi @vishnusarathashling please fork and push to your fork following the standard GitHub development model. See https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request and https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request-from-a-fork

vishnusarathashling commented 2 years ago

Thanks Jonah for sharing the details! @jonahgraham @ilg-ul Please see the PR : https://github.com/eclipse-embed-cdt/eclipse-plugins/pull/513.

ilg-ul commented 2 years ago

Fixed on 2022-04-01 via #513.