Extended Pluggable Monitor specification to allow board-specific settings to be applied (#1855)

* Added methods to obtain specific monitor settings for a board

* Updated documentation

* Implementaion of board-specific monitor settings

* Allow submenu properties to override monitor settings

* Apply code review changes to docs

* Apply suggestions from code review

Co-authored-by: per1234 <[email protected]>

Co-authored-by: per1234 <[email protected]>

Author: cmaglie

Diff for: arduino/cores/board.go

@@ -173,3 +173,9 @@ func (b *Board) IsBoardMatchingIDProperties(query *properties.Map) bool {
 	}
 	return false
 }
+
+// GetMonitorSettings returns the settings for the pluggable monitor of the given protocol
+// and set of board properties.
+func GetMonitorSettings(protocol string, boardProperties *properties.Map) *properties.Map {
+	return boardProperties.SubTree("monitor_port." + protocol)
+}

Diff for: arduino/cores/packagemanager/loader.go

@@ -504,6 +504,7 @@ func (pm *Builder) loadBoards(platform *cores.PlatformRelease) error {
 			convertVidPidIdentificationPropertiesToPluggableDiscovery(boardProperties)
 			convertUploadToolsToPluggableDiscovery(boardProperties)
 		}
+		convertLegacySerialPortRTSDTRSettingsToPluggableMonitor(boardProperties)
 
 		// The board's ID must be available in a board's properties since it can
 		// be used in all configuration files for several reasons, like setting compilation
@@ -523,6 +524,42 @@ func (pm *Builder) loadBoards(platform *cores.PlatformRelease) error {
 	return nil
 }
 
+// Converts the old:
+//
+//   - xxx.serial.disableRTS=true
+//   - xxx.serial.disableDTR=true
+//
+// properties into pluggable monitor compatible:
+//
+//   - xxx.monitor_port.serial.rts=off
+//   - xxx.monitor_port.serial.dtr=off
+func convertLegacySerialPortRTSDTRSettingsToPluggableMonitor(boardProperties *properties.Map) {
+	disabledToOnOff := func(k string) string {
+		if boardProperties.GetBoolean(k) {
+			return "off" // Disabled
+		}
+		return "on" // Not disabled
+	}
+	if boardProperties.ContainsKey("serial.disableDTR") {
+		boardProperties.Set("monitor_port.serial.dtr", disabledToOnOff("serial.disableDTR"))
+	}
+	if boardProperties.ContainsKey("serial.disableRTS") {
+		boardProperties.Set("monitor_port.serial.rts", disabledToOnOff("serial.disableRTS"))
+	}
+	for _, k := range boardProperties.Keys() {
+		if strings.HasSuffix(k, ".serial.disableDTR") {
+			boardProperties.Set(
+				strings.TrimSuffix(k, ".serial.disableDTR")+".monitor_port.serial.dtr",
+				disabledToOnOff(k))
+		}
+		if strings.HasSuffix(k, ".serial.disableRTS") {
+			boardProperties.Set(
+				strings.TrimSuffix(k, ".serial.disableRTS")+".monitor_port.serial.rts",
+				disabledToOnOff(k))
+		}
+	}
+}
+
 // Converts the old:
 //
 //   - xxx.vid.N
@@ -532,7 +569,6 @@ func (pm *Builder) loadBoards(platform *cores.PlatformRelease) error {
 //
 //   - xxx.upload_port.N.vid
 //   - xxx.upload_port.N.pid
-//
 func convertVidPidIdentificationPropertiesToPluggableDiscovery(boardProperties *properties.Map) {
 	n := 0
 	outputVidPid := func(vid, pid string) {

Diff for: arduino/cores/packagemanager/loader_test.go

@@ -106,6 +106,67 @@ arduino_zero_native.pid.3=0x024d
 }`, zero4.Dump())
 }
 
+func TestDisableDTRRTSConversionToPluggableMonitor(t *testing.T) {
+	m, err := properties.LoadFromBytes([]byte(`
+menu.rts=RTS
+menu.dtr=DTR
+foo.menu.rts.on=On
+foo.menu.rts.on.serial.disableRTS=false
+foo.menu.rts.off=Off
+foo.menu.rts.off.serial.disableRTS=true
+foo.menu.dtr.on=On
+foo.menu.dtr.on.serial.disableDTR=false
+foo.menu.dtr.off=Off
+foo.menu.dtr.off.serial.disableDTR=true
+
+arduino_zero.serial.disableDTR=true
+arduino_zero.serial.disableRTS=true
+
+arduino_zero_edbg.serial.disableDTR=false
+arduino_zero_edbg.serial.disableRTS=true
+`))
+	require.NoError(t, err)
+
+	{
+		zero := m.SubTree("arduino_zero")
+		convertLegacySerialPortRTSDTRSettingsToPluggableMonitor(zero)
+		require.Equal(t, `properties.Map{
+  "serial.disableDTR": "true",
+  "serial.disableRTS": "true",
+  "monitor_port.serial.dtr": "off",
+  "monitor_port.serial.rts": "off",
+}`, zero.Dump())
+	}
+	{
+		zeroEdbg := m.SubTree("arduino_zero_edbg")
+		convertLegacySerialPortRTSDTRSettingsToPluggableMonitor(zeroEdbg)
+		require.Equal(t, `properties.Map{
+  "serial.disableDTR": "false",
+  "serial.disableRTS": "true",
+  "monitor_port.serial.dtr": "on",
+  "monitor_port.serial.rts": "off",
+}`, zeroEdbg.Dump())
+	}
+	{
+		foo := m.SubTree("foo")
+		convertLegacySerialPortRTSDTRSettingsToPluggableMonitor(foo)
+		require.Equal(t, `properties.Map{
+  "menu.rts.on": "On",
+  "menu.rts.on.serial.disableRTS": "false",
+  "menu.rts.off": "Off",
+  "menu.rts.off.serial.disableRTS": "true",
+  "menu.dtr.on": "On",
+  "menu.dtr.on.serial.disableDTR": "false",
+  "menu.dtr.off": "Off",
+  "menu.dtr.off.serial.disableDTR": "true",
+  "menu.rts.on.monitor_port.serial.rts": "on",
+  "menu.rts.off.monitor_port.serial.rts": "off",
+  "menu.dtr.on.monitor_port.serial.dtr": "on",
+  "menu.dtr.off.monitor_port.serial.dtr": "off",
+}`, foo.Dump())
+	}
+}
+
 func TestLoadDiscoveries(t *testing.T) {
 	// Create all the necessary data to load discoveries
 	fakePath := paths.New("fake-path")

Diff for: commands/monitor/monitor.go

@@ -67,7 +67,7 @@ func Monitor(ctx context.Context, req *rpc.MonitorRequest) (*PortProxy, *pluggab
 	}
 	defer release()
 
-	m, err := findMonitorForProtocolAndBoard(pme, req.GetPort().GetProtocol(), req.GetFqbn())
+	m, boardSettings, err := findMonitorAndSettingsForProtocolAndBoard(pme, req.GetPort().GetProtocol(), req.GetFqbn())
 	if err != nil {
 		return nil, nil, err
 	}
@@ -82,18 +82,25 @@ func Monitor(ctx context.Context, req *rpc.MonitorRequest) (*PortProxy, *pluggab
 		return nil, nil, &arduino.FailedMonitorError{Cause: err}
 	}
 
-	monIO, err := m.Open(req.GetPort().GetAddress(), req.GetPort().GetProtocol())
-	if err != nil {
-		m.Quit()
-		return nil, nil, &arduino.FailedMonitorError{Cause: err}
-	}
+	// Apply user-requested settings
 	if portConfig := req.GetPortConfiguration(); portConfig != nil {
 		for _, setting := range portConfig.Settings {
+			boardSettings.Remove(setting.SettingId) // Remove board settings overridden by the user
 			if err := m.Configure(setting.SettingId, setting.Value); err != nil {
 				logrus.Errorf("Could not set configuration %s=%s: %s", setting.SettingId, setting.Value, err)
 			}
 		}
 	}
+	// Apply specific board settings
+	for setting, value := range boardSettings.AsMap() {
+		m.Configure(setting, value)
+	}
+
+	monIO, err := m.Open(req.GetPort().GetAddress(), req.GetPort().GetProtocol())
+	if err != nil {
+		m.Quit()
+		return nil, nil, &arduino.FailedMonitorError{Cause: err}
+	}
 
 	logrus.Infof("Port %s successfully opened", req.GetPort().GetAddress())
 	return &PortProxy{
@@ -106,36 +113,39 @@ func Monitor(ctx context.Context, req *rpc.MonitorRequest) (*PortProxy, *pluggab
 	}, descriptor, nil
 }
 
-func findMonitorForProtocolAndBoard(pme *packagemanager.Explorer, protocol, fqbn string) (*pluggableMonitor.PluggableMonitor, error) {
+func findMonitorAndSettingsForProtocolAndBoard(pme *packagemanager.Explorer, protocol, fqbn string) (*pluggableMonitor.PluggableMonitor, *properties.Map, error) {
 	if protocol == "" {
-		return nil, &arduino.MissingPortProtocolError{}
+		return nil, nil, &arduino.MissingPortProtocolError{}
 	}
 
 	var monitorDepOrRecipe *cores.MonitorDependency
+	boardSettings := properties.NewMap()
 
 	// If a board is specified search the monitor in the board package first
 	if fqbn != "" {
 		fqbn, err := cores.ParseFQBN(fqbn)
 		if err != nil {
-			return nil, &arduino.InvalidFQBNError{Cause: err}
+			return nil, nil, &arduino.InvalidFQBNError{Cause: err}
 		}
 
 		_, boardPlatform, _, boardProperties, _, err := pme.ResolveFQBN(fqbn)
 		if err != nil {
-			return nil, &arduino.UnknownFQBNError{Cause: err}
+			return nil, nil, &arduino.UnknownFQBNError{Cause: err}
 		}
 
+		boardSettings = cores.GetMonitorSettings(protocol, boardProperties)
+
 		if mon, ok := boardPlatform.Monitors[protocol]; ok {
 			monitorDepOrRecipe = mon
 		} else if recipe, ok := boardPlatform.MonitorsDevRecipes[protocol]; ok {
 			// If we have a recipe we must resolve it
 			cmdLine := boardProperties.ExpandPropsInString(recipe)
 			cmdArgs, err := properties.SplitQuotedString(cmdLine, `"'`, false)
 			if err != nil {
-				return nil, &arduino.InvalidArgumentError{Message: tr("Invalid recipe in platform.txt"), Cause: err}
+				return nil, nil, &arduino.InvalidArgumentError{Message: tr("Invalid recipe in platform.txt"), Cause: err}
 			}
 			id := fmt.Sprintf("%s-%s", boardPlatform, protocol)
-			return pluggableMonitor.New(id, cmdArgs...), nil
+			return pluggableMonitor.New(id, cmdArgs...), boardSettings, nil
 		}
 	}
 
@@ -150,17 +160,17 @@ func findMonitorForProtocolAndBoard(pme *packagemanager.Explorer, protocol, fqbn
 	}
 
 	if monitorDepOrRecipe == nil {
-		return nil, &arduino.NoMonitorAvailableForProtocolError{Protocol: protocol}
+		return nil, nil, &arduino.NoMonitorAvailableForProtocolError{Protocol: protocol}
 	}
 
 	// If it is a monitor dependency, resolve tool and create a monitor client
 	tool := pme.FindMonitorDependency(monitorDepOrRecipe)
 	if tool == nil {
-		return nil, &arduino.MonitorNotFoundError{Monitor: monitorDepOrRecipe.String()}
+		return nil, nil, &arduino.MonitorNotFoundError{Monitor: monitorDepOrRecipe.String()}
 	}
 
 	return pluggableMonitor.New(
 		monitorDepOrRecipe.Name,
 		tool.InstallDir.Join(monitorDepOrRecipe.Name).String(),
-	), nil
+	), boardSettings, nil
 }

Diff for: commands/monitor/settings.go

@@ -32,7 +32,7 @@ func EnumerateMonitorPortSettings(ctx context.Context, req *rpc.EnumerateMonitor
 	}
 	defer release()
 
-	m, err := findMonitorForProtocolAndBoard(pme, req.GetPortProtocol(), req.GetFqbn())
+	m, boardSettings, err := findMonitorAndSettingsForProtocolAndBoard(pme, req.GetPortProtocol(), req.GetFqbn())
 	if err != nil {
 		return nil, err
 	}
@@ -46,6 +46,19 @@ func EnumerateMonitorPortSettings(ctx context.Context, req *rpc.EnumerateMonitor
 	if err != nil {
 		return nil, &arduino.FailedMonitorError{Cause: err}
 	}
+
+	// Apply default settings for this board and protocol
+	for setting, value := range boardSettings.AsMap() {
+		if param, ok := desc.ConfigurationParameters[setting]; ok {
+			for _, v := range param.Values {
+				if v == value {
+					param.Selected = value
+					break
+				}
+			}
+		}
+	}
+
 	return &rpc.EnumerateMonitorPortSettingsResponse{Settings: convert(desc)}, nil
 }
 

Diff for: docs/platform-specification.md

@@ -903,6 +903,47 @@ will allow all legacy non-pluggable platforms to migrate to pluggable monitor wi
 
 For detailed information, see the [Pluggable Monitor specification](pluggable-monitor-specification.md).
 
+#### Port configuration
+
+Each pluggable monitor has its own default settings that can be overridden using the following board properties:
+
+```
+BOARD_ID.monitor_port.PROTOCOL.SETTING_NAME=SETTING_VALUE
+```
+
+where:
+
+- `BOARD_ID` is the board identifier
+- `PROTOCOL` is the port protocol
+- `SETTING_NAME` and `SETTING_VALUE` are the port setting and the desired value
+
+For example, let's suppose that a board needs the `baudrate` setting of the `serial` port to be `9600`, then the
+corresponding properties in the `boards.txt` file will be:
+
+```
+myboard.monitor_port.serial.baudrate=9600
+```
+
+The settings available in a specific pluggable monitor can be
+[queried directly from it](pluggable-monitor-specification.md#describe-command).
+
+#### Legacy `serial.disableRTS` and `serial.disableDTR` properties
+
+In the old Arduino IDE (<=1.8.x) we used the properties:
+
+```
+BOARD_ID.serial.disableRTS=true
+BOARD_ID.serial.disableDTR=true
+```
+
+to disable RTS and DTR when opening the serial monitor. To keep backward compatibilty the properties above are
+automatically converted to the corresponding pluggable monitor properties:
+
+```
+BOARD_ID.monitor_port.serial.rts=off
+BOARD_ID.monitor_port.serial.dtr=off
+```
+
 ### Verbose parameter
 
 It is possible for the user to enable verbosity from the Preferences panel of the IDEs or Arduino CLI's `--verbose`