Reduce a few LOC in rcS AUTOCNF logic #12467

mcsauder · 2019-07-12T06:03:33Z

Describe problem solved by the proposed pull request
This PR simplifies the rcS script SYS_AUTOCONFIG logic and drops a few LOC. It also moves set/unset statements in the script to the top and bottom as housekeeping and saves about 50 bytes of flash.

Test data / coverage
The logic change is easily examined by inspection. The set/unset copy/paste is also easily examined by inspection.

Describe possible alternatives
If this PR isn't helpful, please feel free to close it!

Thanks!

bkueng · 2019-07-12T07:17:24Z

ROMFS/px4fmu_common/init.d/rcS

 	fi

 	#
 	# Optional board defaults: rc.board_defaults
 	#
-	set BOARD_RC_DEFAULTS /etc/init.d/rc.board_defaults


Keeping the scope of these more local is IMO preferrable, as it makes it easier to track the variable state, and reduces memory usage.

Hi@bkueng! Thanks for your feedback. I understand your point and agree with keeping variables as close as possible to where they are used as a general rule to follow. The trouble with the startup scripts is accounting for variables to ensure they get unset. For variables that get set/unset within a few LOC, this is easy, but for variables that are used by multiple startup files this becomes a headache. I realize that for this PR the set/unsets are close together, I am primarily motivated by employing a standard practice that is easily identifiable and helps reduce errors in the future in this PR.

(It's totally acceptable if you'd prefer to keep the existing. Not a problem at all to close out this PR unmerged, I understand where you are coming from.)

I generally agree, but for these specific instances I prefer the existing way (for the same reasons you already mention).

mcsauder · 2019-07-12T22:20:18Z

Flight tested: https://review.px4.io/plot_app?log=79c9686b-0dad-41d3-bad2-558b5b16ac1c

Bench test:

nsh> param set SYS_AUTOCONFIG 1
+ SYS_AUTOCONFIG: curr: 0 -> new: 1
nsh> reboot
WARN  [shutdown] Reboot �[boot] Rev 0x0 : Ver 0x0 V500
[boot] Fault Log info File No 4 Length 3177 flags:0x01 state:1
[boot] Fault Log is Armed
sercon: Registering CDC/ACM serial driver
sercon: Successfully registered the CDC/ACM serial driver
HW arch: PX4_FMU_V5
HW type: V500
HW version: 0x00000000
HW revision: 0x00000000
FW git-hash: c690ea506928f5603b5c5d54964a3bd82ee78775
FW version: 1.9.0 0 (17367040)
FW git-branch: rcS_cleanup
OS: NuttX
OS version: Release 7.29.0 (119341311)
OS git-hash: a6f5881ea851383877b92b0b6fed1ba9ff9825d0
Build datetime: Jul 12 2019 16:00:22
Build uri: localhost
Toolchain: GNU GCC, 7.2.1 20170904 (release) [ARM/embedded-7-branch revision 255204]
PX4GUID: 0002000000003432333830385115002a001b
MCU: STM32F76xxx, rev. Z
[hardfault_log] Fault Log is Armed
INFO  [tune_control] Publishing standard tune 1
INFO  [param] selected parameter default file /fs/mtd_params
Board defaults: /etc/init.d/rc.board_defaults
INFO  [dataman] Unknown restart, data manager file '/fs/microsd/dataman' size is 362560 bytes
INFO  [px4_work_queue] creating: wq:lp_default, priority: 205, stack: %zu bytes
INFO  [px4_work_queue] creating: wq:I2C1, priority: 248, stack: %zu bytes
rgbled on I2C bus 1 at 0x55 (bus: 100 KHz, max: 100 KHz)
WARN  [rgbled_ncp5623c] no RGB led on bus #1
WARN  [blinkm] I2C init failed
WARN  [blinkm] init failed
INFO  [px4_work_queue] creating: wq:hp_default, priority: 243, stack: %zu bytes
  NAV_ACC_RAD: curr: 10.0000 -> new: 2.0000
  RTL_RETURN_ALT: curr: 60.0000 -> new: 30.0000
  RTL_DESCEND_ALT: curr: 30.0000 -> new: 10.0000
  RTL_LAND_DELAY: curr: -1.0000 -> new: 0.0000
  PWM_MAX: curr: 2000 -> new: 1950
  PWM_MIN: curr: 1000 -> new: 1075
  MC_ROLL_P: curr: 6.5000 -> new: 8.0000
  MC_ROLLRATE_P: curr: 0.1500 -> new: 0.0800
  MC_ROLLRATE_I: curr: 0.2000 -> new: 0.2500
  MC_ROLLRATE_D: curr: 0.0030 -> new: 0.0010
  MC_PITCH_P: curr: 6.5000 -> new: 8.0000
  MC_PITCHRATE_P: curr: 0.1500 -> new: 0.0800
  MC_PITCHRATE_I: curr: 0.2000 -> new: 0.2500
  MC_PITCHRATE_D: curr: 0.0030 -> new: 0.0010
  MC_YAW_P: curr: 2.8000 -> new: 4.0000
  MC_ROLLRATE_MAX: curr: 220.0000 -> new: 1600.0000
  MC_PITCHRATE_MAX: curr: 220.0000 -> new: 1600.0000
  MC_YAWRATE_MAX: curr: 200.0000 -> new: 1000.0000
  MPC_MANTHR_MIN: curr: 0.0800 -> new: 0.0000
  MPC_MAN_TILT_MAX: curr: 35.0000 -> new: 60.0000
  THR_MDL_FAC: curr: 0.0000 -> new: 0.3000
  PWM_RATE: curr: 400 -> new: 0
  SDLOG_PROFILE: curr: 3 -> new: 19
  CBRK_IO_SAFETY: curr: 0 -> new: 22027
* SYS_AUTOCONFIG: curr: 1 -> new: 0
Board sensors: /etc/init.d/rc.board_sensors

EDIT: Set SYS_AUTOCONFIG -> 2 relevant console output:

+ PWM_RATE: curr: 0 -> new: 400
* PWM_RATE: curr: 400 -> new: 0
+ SYS_AUTOCONFIG: curr: 2 -> new: 0

…w LOC checking SYS_AUTOCONFIG with improved logic.

…ly the SYS_AUTOCONFIG logic modification.

mcsauder · 2019-07-16T00:20:12Z

Hi @bkueng ,

I've replaced the set/unset(s), leaving only the SYS_AUTOCONFIG logic modifications. Here are the results from relevant console output from master and this PR, behavior is identical.

PX4:master:

nsh> param set SYS_AUTOCONFIG 1
+ SYS_AUTOCONFIG: curr: 0 -> new: 1
nsh> reboot
...
  NAV_ACC_RAD: curr: 10.0000 -> new: 2.0000
  RTL_RETURN_ALT: curr: 60.0000 -> new: 30.0000
  RTL_DESCEND_ALT: curr: 30.0000 -> new: 10.0000
  RTL_LAND_DELAY: curr: -1.0000 -> new: 0.0000
  PWM_MAX: curr: 2000 -> new: 1950
  PWM_MIN: curr: 1000 -> new: 1075
  MC_ROLL_P: curr: 6.5000 -> new: 8.0000
  MC_ROLLRATE_P: curr: 0.1500 -> new: 0.0800
  MC_ROLLRATE_I: curr: 0.2000 -> new: 0.2500
  MC_ROLLRATE_D: curr: 0.0030 -> new: 0.0010
  MC_PITCH_P: curr: 6.5000 -> new: 8.0000
  MC_PITCHRATE_P: curr: 0.1500 -> new: 0.0800
  MC_PITCHRATE_I: curr: 0.2000 -> new: 0.2500
  MC_PITCHRATE_D: curr: 0.0030 -> new: 0.0010
  MC_YAW_P: curr: 2.8000 -> new: 4.0000
  MC_ROLLRATE_MAX: curr: 220.0000 -> new: 1600.0000
  MC_PITCHRATE_MAX: curr: 220.0000 -> new: 1600.0000
  MC_YAWRATE_MAX: curr: 200.0000 -> new: 1000.0000
  MPC_MANTHR_MIN: curr: 0.0800 -> new: 0.0000
  MPC_MAN_TILT_MAX: curr: 35.0000 -> new: 60.0000
  THR_MDL_FAC: curr: 0.0000 -> new: 0.3000
  PWM_RATE: curr: 400 -> new: 0
  SDLOG_PROFILE: curr: 3 -> new: 19
  CBRK_IO_SAFETY: curr: 0 -> new: 22027
* SYS_AUTOCONFIG: curr: 1 -> new: 0


nsh> param set SYS_AUTOCONFIG 2
+ SYS_AUTOCONFIG: curr: 0 -> new: 2
nsh> reboot
...
+ PWM_RATE: curr: 0 -> new: 400
* PWM_RATE: curr: 400 -> new: 0
+ SYS_AUTOCONFIG: curr: 2 -> new: 0

This PR:

nsh> param set SYS_AUTOCONFIG 1
+ SYS_AUTOCONFIG: curr: 0 -> new: 1
nsh> reboot
...
  NAV_ACC_RAD: curr: 10.0000 -> new: 2.0000
  RTL_RETURN_ALT: curr: 60.0000 -> new: 30.0000
  RTL_DESCEND_ALT: curr: 30.0000 -> new: 10.0000
  RTL_LAND_DELAY: curr: -1.0000 -> new: 0.0000
  PWM_MAX: curr: 2000 -> new: 1950
  PWM_MIN: curr: 1000 -> new: 1075
  MC_ROLL_P: curr: 6.5000 -> new: 8.0000
  MC_ROLLRATE_P: curr: 0.1500 -> new: 0.0800
  MC_ROLLRATE_I: curr: 0.2000 -> new: 0.2500
  MC_ROLLRATE_D: curr: 0.0030 -> new: 0.0010
  MC_PITCH_P: curr: 6.5000 -> new: 8.0000
  MC_PITCHRATE_P: curr: 0.1500 -> new: 0.0800
  MC_PITCHRATE_I: curr: 0.2000 -> new: 0.2500
  MC_PITCHRATE_D: curr: 0.0030 -> new: 0.0010
  MC_YAW_P: curr: 2.8000 -> new: 4.0000
  MC_ROLLRATE_MAX: curr: 220.0000 -> new: 1600.0000
  MC_PITCHRATE_MAX: curr: 220.0000 -> new: 1600.0000
  MC_YAWRATE_MAX: curr: 200.0000 -> new: 1000.0000
  MPC_MANTHR_MIN: curr: 0.0800 -> new: 0.0000
  MPC_MAN_TILT_MAX: curr: 35.0000 -> new: 60.0000
  THR_MDL_FAC: curr: 0.0000 -> new: 0.3000
  PWM_RATE: curr: 400 -> new: 0
  SDLOG_PROFILE: curr: 3 -> new: 19
  CBRK_IO_SAFETY: curr: 0 -> new: 22027
* SYS_AUTOCONFIG: curr: 1 -> new: 0
...
nsh> param set SYS_AUTOCONFIG 2
+ SYS_AUTOCONFIG: curr: 0 -> new: 2
nsh> reboot
...
+ PWM_RATE: curr: 0 -> new: 400
* PWM_RATE: curr: 400 -> new: 0
+ SYS_AUTOCONFIG: curr: 2 -> new: 0

bkueng · 2019-07-16T06:14:47Z

ROMFS/px4fmu_common/init.d/rcS

 		then
-			set AUTOCNF yes
-		else
-			set AUTOCNF no


I don't see this set to no in the else case anymore. If not set, it could cause problems with script evaluation.

Thanks for catching that. That deletion shouldn't have occurred. The unset remains at the bottom of the script, but the script still lacks a single design pattern in that regard. By the logic we've discussed above the set/unset for AUTOCNF should probably be moved to this location, but then we'd slowly end up where we were last year with variables not getting unset. My preference would still be a single rule to follow.

mcsauder · 2019-07-16T14:51:16Z

Thanks @bkueng !

bkueng reviewed Jul 12, 2019

View reviewed changes

mcsauder force-pushed the rcS_cleanup branch from 33dc640 to c690ea5 Compare July 12, 2019 22:18

mcsauder force-pushed the rcS_cleanup branch from c690ea5 to b21f58b Compare July 13, 2019 04:34

weekly-digest bot mentioned this pull request Jul 14, 2019

Weekly Digest (7 July, 2019 - 14 July, 2019) #12474

Closed

mcsauder added 2 commits July 15, 2019 18:10

Set/unset rcS vars at beginning and end of rcS script and reduce a fe…

0480e70

…w LOC checking SYS_AUTOCONFIG with improved logic.

Restore current placement of set/unset vars in rcS script to leave on…

a0562f0

…ly the SYS_AUTOCONFIG logic modification.

mcsauder force-pushed the rcS_cleanup branch from b21f58b to a0562f0 Compare July 16, 2019 00:10

bkueng reviewed Jul 16, 2019

View reviewed changes

Replace set AUTOCNF no after inadvertent deletion.

f42e4c5

mcsauder changed the title ~~Set/unset rcS vars at beginning and end of rcS script and reduce a few LOC~~ Reduce a few LOC in rcS AUTOCNF logic Jul 16, 2019

bkueng approved these changes Jul 16, 2019

View reviewed changes

bkueng merged commit 4a02475 into PX4:master Jul 16, 2019

mcsauder deleted the rcS_cleanup branch July 16, 2019 18:21

weekly-digest bot mentioned this pull request Jul 21, 2019

Weekly Digest (14 July, 2019 - 21 July, 2019) #12528

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce a few LOC in rcS AUTOCNF logic #12467

Reduce a few LOC in rcS AUTOCNF logic #12467

mcsauder commented Jul 12, 2019

bkueng Jul 12, 2019

mcsauder Jul 12, 2019

mcsauder Jul 12, 2019

bkueng Jul 15, 2019

mcsauder commented Jul 12, 2019 •

edited

Loading

mcsauder commented Jul 16, 2019

bkueng Jul 16, 2019

mcsauder Jul 16, 2019

mcsauder commented Jul 16, 2019

Reduce a few LOC in rcS AUTOCNF logic #12467

Reduce a few LOC in rcS AUTOCNF logic #12467

Conversation

mcsauder commented Jul 12, 2019

bkueng Jul 12, 2019

Choose a reason for hiding this comment

mcsauder Jul 12, 2019

Choose a reason for hiding this comment

mcsauder Jul 12, 2019

Choose a reason for hiding this comment

bkueng Jul 15, 2019

Choose a reason for hiding this comment

mcsauder commented Jul 12, 2019 • edited Loading

mcsauder commented Jul 16, 2019

bkueng Jul 16, 2019

Choose a reason for hiding this comment

mcsauder Jul 16, 2019

Choose a reason for hiding this comment

mcsauder commented Jul 16, 2019

mcsauder commented Jul 12, 2019 •

edited

Loading