Support disable/enable hardware breakpoint #89

trongle0504 · 2023-04-28T07:32:20Z

Currently, enabling hardwareBreakpoint in launch.json argument allows the debugger to set hardware break-point or software break-point only ("hardwareBreakpoint": true). We need to support enabling/disabling setting SW and HW break-point simultaneously while debugging.

eclipse-cdt-bot · 2023-04-28T07:36:01Z

Can one of the admins verify this patch?

trongle0504 · 2023-04-28T08:40:14Z

Hi @jonahgraham -san,

Could you please help me review the code?
Please see the document attached to ticket IDE-63527 for details.

I also commit code in repository cdt-gdb-adapter.
eclipse-cdt-cloud/cdt-gdb-adapter#264

Thank you!

jonahgraham

As mentioned in eclipse-cdt-cloud/cdt-gdb-adapter#264 using a toggle for the new command is problematic as the initial state of hw breakpoint as set in launch.json is not taken into account here and leaves the UI commands in an inconsistent state.

I have serious concerns about the overall usability of this design and I hope that once you share the information in the below mentioned document I can understand the workflows.

For example, are you supposed to be able to change hw -> sw (or back) in the middle of a debug session?

Please see the document attached to ticket IDE-63527 for details.

Please provide required information in the open - this is an open source project so should not require access to private information to understand this.

jonahgraham · 2023-04-28T13:37:43Z

src/breakpoint/DisableHWBreakpoint.ts

+                async () => {
+                    const session = vscode.debug.activeDebugSession;
+                    if (session) {
+                        await vscode.commands.executeCommand(


Can you simplify this to a single context?

The solution has been changed to use Toggle Breakpoint

jonahgraham · 2023-04-28T13:38:22Z

src/breakpoint/DisableHWBreakpoint.ts

+                            'enableHardware',
+                            true
+                        );
+                        await session.customRequest(


Please pass the desired state to the adapter.

Since using Toggle Breakpoint, I think there is no need to send status in customRequest

jonahgraham · 2023-04-28T13:39:00Z

package.json

+        {
+          "command": "cdt.debug.enableHWBreakpoint",
+          "group": "navigation",
+          "when": "(!enableHardware && !disableHardware) || enableHardware"


Having enableHardware twice in this expression is unexpected.

The solution has been changed to use Toggle Breakpoint

trongle0504 · 2023-05-05T08:36:27Z

As mentioned in eclipse-cdt-cloud/cdt-gdb-adapter#264 using a toggle for the new command is problematic as the initial state of hw breakpoint as set in launch.json is not taken into account here and leaves the UI commands in an inconsistent state.

I have serious concerns about the overall usability of this design and I hope that once you share the information in the below mentioned document I can understand the workflows.

For example, are you supposed to be able to change hw -> sw (or back) in the middle of a debug session?

Please see the document attached to ticket IDE-63527 for details.

Please provide required information in the open - this is an open source project so should not require access to private information to understand this.

Hi @jonahgraham ,

I have some changes in the purpose of this PR compared to the original one.

Below is the description of this PR:

The purpose of this PR is to support toggle breakpoint being able to change the Hardware breakpoint -> Software breakpoint (or back) in the middle of a debug session.

Case 1: If hardwareBreakpoint is not configured in launch.json, the default setting will be SW breakpoint.

After launching the debugger, the user right-clicks on the lineNumber and selects Toggle Breakpoint.
The Debug Console will display a message indicating that HW breakpoint is now in use.
The user can continue to use Toggle Breakpoint to switch between HW and SW breakpoints.

Case 2: If "hardwareBreakpoint": true is configured in launch.json, the default setting will be HW breakpoint.

After launching the debugger, the user right-clicks on the lineNumber and selects Toggle Breakpoint.
The Debug Console will display a message indicating that the SW breakpoint is now in use.
The user can continue to use Toggle Breakpoint to switch between HW and SW breakpoints.

Could you please help me review it again?

Thank you!

jonahgraham

The purpose of this PR is to support toggle breakpoint being able to change the Hardware breakpoint -> Software breakpoint (or back) in the middle of a debug session.

As you have implemented it now the toggle will apply to future breakpoints that are inserted by GDB - that intention is not obvious to the user. Was there an intention to change existing breakpoints from hw -> sw (or vice versa)?

Messages in the Debug Console are somewhat useful, but they can also be lost among lots of other output from the backend and it isn't visible if the Debug Console is not open.

VS Code has standard ways of notifying users. Please look at showInformationMessage and review UX guidelines on notifications.

However in this case there is another problem. The current implementation provides no way to show user the current setting, leading to the user having to toggle the setting twice and looking at Debug Console to know the current setting.

Perhaps using the status bar to show current state is useful? Or a checkmark next to a menu item?

The final problem I have with this solution is that there is no way to show users for already inserted breakpoints whether it is a hardware or software breakpoint - other than user typing >info break or similar in the CLI.

Note that I can accept a solution that does not address every UX items I have described above as some of them are really hard and may require changes to VSCode too. However the current solution is simply too far away from an intuitive design for users.

jonahgraham · 2023-05-05T12:47:07Z

package.json

+      },
+      {
+        "command": "cdt.debug.toggleBreakpoint",
+        "title": "Toggle Breakpoint"


"Toggle breakpoint" implies that you are setting and unsetting a breakpoint at that line (when using context menu)

A better term would be "Use Hardware breakpoints" with a checkmark or something that indicates that this is command is related to hardware breakpoints.

I updated name to "Use Hardware breakpoints"

jonahgraham · 2023-05-05T12:49:00Z

package.json

@@ -503,6 +508,12 @@
          "group": "navigation",
          "when": "debugType == amalgamator"
        }
+      ],
+      "editor/lineNumber/context": [


By making this available (only or primarily) in the line number context it implies that the option applies to the line or the breakpoint on the line.

But that isn't what happens in your current implementation of the backend. The toggling means that future breakpoints that are inserted are hardware.

I changed the way it is displayed when the user uses "Use Hardware breakpoints". Switching from "editor/lineNumber/context" to using "editor/context".

jonahgraham · 2023-05-05T12:49:57Z

src/breakpoint/ToggleBreakpoint.ts

+                    const session = vscode.debug.activeDebugSession;
+                    if (session) {
+                        await session.customRequest(
+                            'cdt-gdb-adapter/toggleBreakpoint'


If you conclude that the front end really presents as a toggle, perhaps adding a get would be a good idea, then you can get current state and change it.

I created customRequest() to get the current state, then change the state and send it to set HW Breakpoint.

jonahgraham · 2023-05-05T13:07:14Z

package.json

@@ -16,13 +16,14 @@
  "homepage": "https://github.com/eclipse-cdt-cloud/cdt-gdb-vscode#readme",
  "main": "./out/extension",
  "engines": {
-    "vscode": "^1.76.0"
+    "vscode": "^1.78.0"


Normally I would want version change like this to be split out - but since there is a dependency here on microsoft/vscode#175945 I think it is ok this time.

Is this version bump still needed since you aren't using new feature? Lets split out the version bump to a new PR.

Remove ToggleBreakpoint.ts to update new code

Update new class to getHWBreakpoint, setHWBreakpoint, and update status to Status Bar

Change the name accordingly

trongle0504 · 2023-05-10T06:40:59Z

Messages in the Debug Console are somewhat useful, but they can also be lost among lots of other output from the backend and it isn't visible if the Debug Console is not open.

VS Code has standard ways of notifying users. Please look at showInformationMessage and review UX guidelines on notifications.

I replaced the message at the Debug console to the one using VSCode's Notifications.

trongle0504 · 2023-05-10T06:57:15Z

However in this case there is another problem. The current implementation provides no way to show user the current setting, leading to the user having to toggle the setting twice and looking at Debug Console to know the current setting.

Perhaps using the status bar to show current state is useful? Or a checkmark next to a menu item?

I used the status bar to show the current state of the HW breakpoint.
Currently, the status is only visible when the user starts debugging, and when debugging is finished it will automatically hide.

When the user starts debugging:
When the user disconnects debugging:

The state also updates properly when the user configures "hardwareBreakpoint": true, "hardwareBreakpoint": false or even unconfigured "hardwareBreakpoint" at the launch.json file.

trongle0504 · 2023-05-10T07:09:11Z

As you have implemented it now the toggle will apply to future breakpoints that are inserted by GDB - that intention is not obvious to the user. Was there an intention to change existing breakpoints from hw -> sw (or vice versa)?

With current update user can change from HW breakpoint -> SW breakpoint and switch from SW breakpoint -> HW breakpoint

trongle0504 · 2023-05-10T07:14:27Z

Hi @jonahgraham ,
I've updated some of the changes you suggested including:

Changed notify when setting HW breakpoint for users by standard ways of notifying users of VSCode
Show current status of the HW breakpoint to user at status bar.
Change the name "Toggle breakpoints" to the name "Use Hardware breakpoints".
Use cdt-gdb-adapter/getHWBreakpoint, cdt-gdb-adapter/setHWBreakpoint to get and set HW state breakpoint to GDB using customRequest()

Could you please help me review it again?

Thank you!

trongle0504 · 2023-05-10T07:17:18Z

The final problem I have with this solution is that there is no way to show users for already inserted breakpoints whether it is a hardware or software breakpoint - other than user typing >info break or similar in the CLI.

Regarding this issue, I have investigated but I have no solution for this.
Any ideas for this issue? or we will improve it in the future.

Change the display when the user uses "Use Hardware breakpoints".

jonahgraham

Some of the language needs some additional thought - i.e differentiating between action and state. We can refine that once we agree on the high level UX issues like below.

The final problem I have with this solution is that there is no way to show users for already inserted breakpoints whether it is a hardware or software breakpoint - other than user typing >info break or similar in the CLI.

Regarding this issue, I have investigated but I have no solution for this. Any ideas for this issue? or we will improve it in the future.

I need some more time to contemplate this aspect. It is related to other issues like preserving settings and setting expectations for users. Because there is no persistence and no way of identifying which type of breakpoint is used I fear we are giving users a false expectation.

Consider for example this flow:

User sets use hw breakpoints in launch config
User inserts some hardware breakpoints
User starts debug session
User toggles to use software breakpoints
User inserts their software breakpoints
User presses "Toggle Active Breakpoints" (twice)
Now all breakpoints are software breakpoints

A similar situation:

User sets use hw breakpoints in launch config
User inserts some hardware breakpoints
User starts debug session
User toggles to use software breakpoints
User inserts their software breakpoints
User restarts their debug sessions
Now all breakpoints are hardware breakpoints (and likely many are not successfully inserted due to limitation on number of hardware breakpoints)

jonahgraham · 2023-05-10T13:19:24Z

package.json

@@ -16,13 +16,14 @@
  "homepage": "https://github.com/eclipse-cdt-cloud/cdt-gdb-vscode#readme",
  "main": "./out/extension",
  "engines": {
-    "vscode": "^1.76.0"
+    "vscode": "^1.78.0"


Is this version bump still needed since you aren't using new feature? Lets split out the version bump to a new PR.

jonahgraham · 2023-05-10T13:20:36Z

src/breakpoint/BreakpointStatusBarItem.ts

@@ -0,0 +1,23 @@
+import { window, StatusBarAlignment, StatusBarItem } from 'vscode';


Suggested change

import { window, StatusBarAlignment, StatusBarItem } from 'vscode';

/*********************************************************************

* Copyright (c) 2023 Renesas Electronics Corporation and others

*

* This program and the accompanying materials are made

* available under the terms of the Eclipse Public License 2.0

* which is available at https://www.eclipse.org/legal/epl-2.0/

*

* SPDX-License-Identifier: EPL-2.0

*********************************************************************/

import { window, StatusBarAlignment, StatusBarItem } from 'vscode';

jonahgraham · 2023-05-10T13:20:56Z

src/breakpoint/SetHWBreakpoint.ts

@@ -0,0 +1,78 @@
+import { ExtensionContext, window } from 'vscode';


Suggested change

import { ExtensionContext, window } from 'vscode';

/*********************************************************************

* Copyright (c) 2023 Renesas Electronics Corporation and others

*

* This program and the accompanying materials are made

* available under the terms of the Eclipse Public License 2.0

* which is available at https://www.eclipse.org/legal/epl-2.0/

*

* SPDX-License-Identifier: EPL-2.0

*********************************************************************/

import { ExtensionContext, window } from 'vscode';

jonahgraham · 2023-05-10T13:21:46Z

src/breakpoint/BreakpointStatusBarItem.ts

@@ -0,0 +1,23 @@
+import { window, StatusBarAlignment, StatusBarItem } from 'vscode';
+
+export class BreakpointStatusBarItem {


What is the purpose of this class? Why not use createStatusBarItem directly?

jonahgraham · 2023-05-10T13:23:58Z

src/breakpoint/SetHWBreakpoint.ts

+                        vscode.Uri.file(configPath)
+                    );
+                const HWBreakpointParameter: boolean | undefined =
+                    config?.configurations[0]?.hardwareBreakpoint;


Is it possible to get the current status from the backend rather than trying to maintain duplicate logic in the frontend?

trongle0504 · 2023-09-06T09:06:52Z

I'm sorry, I would like to close this request because there are some complex cases to cover, and it may cause inconvenience for users. Thank you for taking the time to review the code.

trongle0504 added 2 commits April 28, 2023 14:23

Support disable/enable hardware breakpoint

afcb1ec

Support disable/enable hardware breakpoint

faaeebe

trongle0504 mentioned this pull request Apr 28, 2023

Support disable/enable hardware breakpoint eclipse-cdt-cloud/cdt-gdb-adapter#264

Closed

jonahgraham requested changes Apr 28, 2023

View reviewed changes

trongle0504 added 6 commits May 5, 2023 14:45

Delete DisableHWBreakpoint.ts

8b487b0

Delete EnableHWBreakpoint.ts

792f80d

Add ToggleBreakpoint.ts

895ef15

Update ToggleBreakpoint

f198f97

Update ToggleBreakpoint

c750341

Update VSCode Version

a1de0ca

trongle0504 requested a review from jonahgraham May 5, 2023 08:41

jonahgraham requested changes May 5, 2023

View reviewed changes

trongle0504 added 4 commits May 10, 2023 11:02

Remove ToggleBreakpoint.ts

64295ed

Remove ToggleBreakpoint.ts to update new code

Update SetHWBreakpoint

ae89dd5

Update new class to getHWBreakpoint, setHWBreakpoint, and update status to Status Bar

Update package.json

cbde64b

Change the name accordingly

Update extension.ts

77b7156

Change the name accordingly

Update package.json

55f71c1

Change the display when the user uses "Use Hardware breakpoints".

trongle0504 requested a review from jonahgraham May 10, 2023 07:39

jonahgraham requested changes May 10, 2023

View reviewed changes

trongle0504 closed this Sep 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support disable/enable hardware breakpoint #89

Support disable/enable hardware breakpoint #89

trongle0504 commented Apr 28, 2023

eclipse-cdt-bot commented Apr 28, 2023

trongle0504 commented Apr 28, 2023

jonahgraham left a comment

jonahgraham Apr 28, 2023

trongle0504 May 5, 2023

jonahgraham Apr 28, 2023

trongle0504 May 5, 2023

jonahgraham Apr 28, 2023

trongle0504 May 5, 2023

trongle0504 commented May 5, 2023

jonahgraham left a comment

jonahgraham May 5, 2023

trongle0504 May 10, 2023

jonahgraham May 5, 2023

trongle0504 May 10, 2023

jonahgraham May 5, 2023

trongle0504 May 10, 2023

jonahgraham May 5, 2023

jonahgraham May 10, 2023

trongle0504 commented May 10, 2023

trongle0504 commented May 10, 2023 •

edited

Loading

trongle0504 commented May 10, 2023

trongle0504 commented May 10, 2023

trongle0504 commented May 10, 2023

jonahgraham left a comment

jonahgraham May 10, 2023

jonahgraham May 10, 2023

jonahgraham May 10, 2023

jonahgraham May 10, 2023

jonahgraham May 10, 2023

trongle0504 commented Sep 6, 2023

		@@ -0,0 +1,23 @@
		import { window, StatusBarAlignment, StatusBarItem } from 'vscode';

-import { window, StatusBarAlignment, StatusBarItem } from 'vscode';
+/*********************************************************************
+ * Copyright (c) 2023 Renesas Electronics Corporation and others
+ *
+ * This program and the accompanying materials are made
+ * available under the terms of the Eclipse Public License 2.0
+ * which is available at https://www.eclipse.org/legal/epl-2.0/
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ *********************************************************************/
+import { window, StatusBarAlignment, StatusBarItem } from 'vscode';

		@@ -0,0 +1,78 @@
		import { ExtensionContext, window } from 'vscode';

		@@ -0,0 +1,23 @@
		import { window, StatusBarAlignment, StatusBarItem } from 'vscode';

		export class BreakpointStatusBarItem {

Support disable/enable hardware breakpoint #89

Support disable/enable hardware breakpoint #89

Conversation

trongle0504 commented Apr 28, 2023

eclipse-cdt-bot commented Apr 28, 2023

trongle0504 commented Apr 28, 2023

jonahgraham 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

trongle0504 commented May 5, 2023

jonahgraham 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

Choose a reason for hiding this comment

trongle0504 commented May 10, 2023

trongle0504 commented May 10, 2023 • edited Loading

trongle0504 commented May 10, 2023

trongle0504 commented May 10, 2023

trongle0504 commented May 10, 2023

jonahgraham 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

trongle0504 commented Sep 6, 2023

trongle0504 commented May 10, 2023 •

edited

Loading