Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Optionally disable SF1xx sensor in forward flight #22369

Merged
merged 1 commit into from
Dec 13, 2023
Merged

Optionally disable SF1xx sensor in forward flight #22369

merged 1 commit into from
Dec 13, 2023

Conversation

niklaut
Copy link
Contributor

@niklaut niklaut commented Nov 14, 2023 •
edited

Adds a parameter SF1XX_MODE that controls if the SF1xx distance sensor will disable automatically in forward flight after a commander transition.

Test coverage

Tested in hardware via NSH commands commander transition and param set.

Context

Example Behavior 
nsh> lightware_laser_i2c start -X
lightware_laser_i2c #0 on I2C bus 1 (external) address 0x66 rotation 25
nsh> commander transition
INFO  [lightware_laser_i2c] Emission Control: disabling sensor!
nsh> commander transition
INFO  [lightware_laser_i2c] Emission Control: enabling sensor!
nsh> commander transition
INFO  [lightware_laser_i2c] Emission Control: disabling sensor!
nsh> param set SF1XX_MODE 1
+ SF1XX_MODE: curr: 2 -> new: 1
INFO  [lightware_laser_i2c] Emission Control: enabling sensor!
nsh> param set SF1XX_MODE 0
  SF1XX_MODE: curr: 1 -> new: 0
INFO  [lightware_laser_i2c] Emission Control: disabling sensor!
nsh> commander transition
nsh> commander transition
nsh> param set SF1XX_MODE 2
+ SF1XX_MODE: curr: 0 -> new: 2
nsh> commander transition
INFO  [lightware_laser_i2c] Emission Control: enabling sensor!
nsh> param set SF1XX_MODE 0
+ SF1XX_MODE: curr: 2 -> new: 0
INFO  [lightware_laser_i2c] Emission Control: disabling sensor!
nsh> param set SF1XX_MODE 2
+ SF1XX_MODE: curr: 0 -> new: 2
INFO  [lightware_laser_i2c] Emission Control: enabling sensor!
nsh> param set SF1XX_MODE 1
+ SF1XX_MODE: curr: 2 -> new: 1
nsh> commander transition
nsh> param set SF1XX_MODE 1param set SF1XX_MODE 2
  SF1XX_MODE: curr: 1 -> new: 2
INFO  [lightware_laser_i2c] Emission Control: disabling sensor!
nsh> param set SF1XX_MODE 1
+ SF1XX_MODE: curr: 2 -> new: 1
INFO  [lightware_laser_i2c] Emission Control: enabling sensor!
nsh> param set SF1XX_MODE 2
  SF1XX_MODE: curr: 1 -> new: 2
INFO  [lightware_laser_i2c] Emission Control: disabling sensor!
nsh> commander transition
INFO  [lightware_laser_i2c] Emission Control: enabling sensor!

Changelog Entry

For release notes:

Optionally disable SF1xx distance sensor in forward flight.

@niklaut niklaut marked this pull request as ready for review November 15, 2023 10:31
MaEtUgR
MaEtUgR requested changes Nov 16, 2023
View reviewed changes
Copy link
Member

@MaEtUgR MaEtUgR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have two general questions:

  1. Is it worth to schedule a separate module just to read a parameter and based on its value and the VTOL state publish a completely separate message?
  2. Do we have a use case and can distinguish all these frequencies "radio", "microwave", "IR", "light", "UV" ?

I'm asking because that's a lot of overhead to optionally stop distance sensors (or radios?) in fast-forward flight. I would just condense the class into a library that specifically does the required logic and use it in the driver that supports the functionality. This to avoid consuming resources for module, message, strings, second parameter. I'm happy to discuss.

src/modules/emission_control/emcon.cpp Outdated Show resolved Hide resolved
@niklaut
Copy link
Contributor Author

niklaut commented Nov 22, 2023 •
edited

Is it worth to schedule a separate module just to read a parameter and based on its value and the VTOL state publish a completely separate message?

I was going for a more holistic solution rather than a specific patch, since I wanted to decouple the state detection (VTOL<->forward flight) from the actual restriction, so that it is easier to extend to other states and drivers as well.

Do we have a use case and can distinguish all these frequencies "radio", "microwave", "IR", "light", "UV" ?

No, currently only IR restrictions are implemented. I can remove the rest if you want. However, I think this mask is a good starting point for future expansion. I don't really see how disabling IR, but not light emissions, is going to be useful for example.

I would just condense the class into a library that specifically does the required logic and use it in the driver that supports the functionality.

I wanted the emcon module to be optional via Kconfig, I wasn't sure how to do that for libraries (just… not depend on them? but how do I configure that then?).
And I specifically wanted to use uORB to prevent more synchronization mutexes being used, since the drivers can be on different workqueues.
So that lead me to this design.

This to avoid consuming resources for module, message, strings, second parameter.

Yes, therefore I used the macros to remove the uORB subscription from the drivers when the EMCON module is not used.

Resource Usage with these changes:

Memory region         Used Size  Region Size  %age Used
      FLASH_ITCM:          0 GB      2016 KB      0.00%
      FLASH_AXIM:     2060213 B      2016 KB     99.80%
        ITCM_RAM:          0 GB        16 KB      0.00%
        DTCM_RAM:          0 GB       128 KB      0.00%
           SRAM1:       92572 B       368 KB     24.57%
           SRAM2:          0 GB        16 KB      0.00%

Resource Usage with these changes, but CONFIG_MODULES_EMISSION_CONTROL=n:

Memory region         Used Size  Region Size  %age Used
      FLASH_ITCM:          0 GB      2016 KB      0.00%
      FLASH_AXIM:     2057265 B      2016 KB     99.66%
        ITCM_RAM:          0 GB        16 KB      0.00%
        DTCM_RAM:          0 GB       128 KB      0.00%
           SRAM1:       92572 B       368 KB     24.57%
           SRAM2:          0 GB        16 KB      0.00%

(2057265 - 2057225 = 40B increase)

Resource Usage without these changes:

Memory region         Used Size  Region Size  %age Used
      FLASH_ITCM:          0 GB      2016 KB      0.00%
      FLASH_AXIM:     2057225 B      2016 KB     99.65%
        ITCM_RAM:          0 GB        16 KB      0.00%
        DTCM_RAM:          0 GB       128 KB      0.00%
           SRAM1:       92572 B       368 KB     24.57%
           SRAM2:          0 GB        16 KB      0.00%

so a 2060213 - 2057225 = 2988B flash increase. Not that great.

@niklaut
Copy link
Contributor Author

niklaut commented Nov 22, 2023

I've quickly moved the emission control logic into the driver itself just to see how much overhead it is at minimum.
(This code is untested).

Memory region         Used Size  Region Size  %age Used
      FLASH_ITCM:          0 GB      2016 KB      0.00%
      FLASH_AXIM:     2057633 B      2016 KB     99.67%
        ITCM_RAM:          0 GB        16 KB      0.00%
        DTCM_RAM:          0 GB       128 KB      0.00%
           SRAM1:       92572 B       368 KB     24.57%
           SRAM2:          0 GB        16 KB      0.00%

So minimum overhead is 2057633 - 2057225 = 408B.

If you like this more, then I can polish and test it.

MaEtUgR
MaEtUgR reviewed Nov 22, 2023
View reviewed changes
Copy link
Member

@MaEtUgR MaEtUgR left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thank you! That's much better in my eyes. I know it's conceptually not future proof but I'd build out the more generic approach as soon as there are use cases. Sorry for that. Be sure to run make format and possibly add the pre-commit hook for style checks.

src/drivers/distance_sensor/lightware_laser_i2c/lightware_laser_i2c.cpp Outdated Show resolved Hide resolved
src/drivers/distance_sensor/lightware_laser_i2c/parameters.c Outdated Show resolved Hide resolved
src/drivers/distance_sensor/lightware_laser_i2c/lightware_laser_i2c.cpp Outdated Show resolved Hide resolved
@niklaut niklaut changed the title Add Emission Control module Optionally disable SF1xx sensor in forward flight Nov 22, 2023
@niklaut
Copy link
Contributor Author

niklaut commented Nov 22, 2023

Ok, now this solution is also tested and formatted properly.

MaEtUgR
MaEtUgR reviewed Nov 24, 2023
View reviewed changes
Copy link
Member

@MaEtUgR MaEtUgR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nit-picking. Otherwise it looks good.

src/drivers/distance_sensor/lightware_laser_i2c/lightware_laser_i2c.cpp Outdated Show resolved Hide resolved
src/drivers/distance_sensor/lightware_laser_i2c/lightware_laser_i2c.cpp Outdated Show resolved Hide resolved
@niklaut niklaut force-pushed the feature/emcon branch 2 times, most recently from d8a5031 to 491379c Compare November 24, 2023 12:27
@niklaut
Copy link
Contributor Author

niklaut commented Nov 24, 2023

Updated

@niklaut niklaut force-pushed the feature/emcon branch 2 times, most recently from 0f72268 to 851a086 Compare November 24, 2023 15:09
@niklaut
Copy link
Contributor Author

niklaut commented Nov 24, 2023

Unfortunately I didn't find a camera without an IR filter for 900nm, but I did add the laser control command to turn off the laser. I tested this by allowing the collection to continue while the laser is disabled to check the return value, which was -1 when the laser was disabled.

nsh> param set SF1XX_MODE 0
  SF1XX_MODE: curr: 1 -> new: 0
INFO  [lightware_laser_i2c] Emission Control: disabling sensor!
nsh> listener distance_sensor

TOPIC: distance_sensor
 distance_sensor
    timestamp: 154281632 (0.008059 seconds ago)
    device_id: 7562761 (Type: 0x73, I2C:1 (0x66))
    min_distance: 0.20000
    max_distance: 100.00000
    current_distance: -1.00000
    variance: 0.00000
    h_fov: 0.00000
    v_fov: 0.00000
    q: [0.00000, 0.00000, 0.00000, 0.00000] (Roll: 0.0 deg, Pitch: -0.0 deg, Yaw: 0.0 deg)
    signal_quality: 0
    type: 0
    orientation: 25

nsh> param set SF1XX_MODE 2
+ SF1XX_MODE: curr: 0 -> new: 2
INFO  [lightware_laser_i2c] Emission Control: enabling sensor!
nsh> listener distance_sensor

TOPIC: distance_sensor
 distance_sensor
    timestamp: 166089923 (0.011141 seconds ago)
    device_id: 7562761 (Type: 0x73, I2C:1 (0x66))
    min_distance: 0.20000
    max_distance: 100.00000
    current_distance: 1.02000
    variance: 0.00000
    h_fov: 0.00000
    v_fov: 0.00000
    q: [0.00000, 0.00000, 0.00000, 0.00000] (Roll: 0.0 deg, Pitch: -0.0 deg, Yaw: 0.0 deg)
    signal_quality: 100
    type: 0
    orientation: 25

nsh> commander transition
INFO  [lightware_laser_i2c] Emission Control: disabling sensor!
nsh> listener distance_sensor

TOPIC: distance_sensor
 distance_sensor
    timestamp: 176114507 (0.020742 seconds ago)
    device_id: 7562761 (Type: 0x73, I2C:1 (0x66))
    min_distance: 0.20000
    max_distance: 100.00000
    current_distance: -1.00000
    variance: 0.00000
    h_fov: 0.00000
    v_fov: 0.00000
    q: [0.00000, 0.00000, 0.00000, 0.00000] (Roll: 0.0 deg, Pitch: -0.0 deg, Yaw: 0.0 deg)
    signal_quality: 0
    type: 0
    orientation: 25

nsh> commander transition
WARN  [health_and_arming_checks] Preflight Fail: Airspeed invalid
ERROR [health_and_arming_checks] Low battery level
INFO  [lightware_laser_i2c] Emission Control: enabling sensor!
nsh> listener distance_sensor

TOPIC: distance_sensor
 distance_sensor
    timestamp: 180570472 (0.018516 seconds ago)
    device_id: 7562761 (Type: 0x73, I2C:1 (0x66))
    min_distance: 0.20000
    max_distance: 100.00000
    current_distance: 1.36000
    variance: 0.00000
    h_fov: 0.00000
    v_fov: 0.00000
    q: [0.00000, 0.00000, 0.00000, 0.00000] (Roll: 0.0 deg, Pitch: -0.0 deg, Yaw: 0.0 deg)
    signal_quality: 100
    type: 0
    orientation: 25

@sfuhrer
Copy link
Contributor

sfuhrer commented Dec 12, 2023

@MaEtUgR @bkueng could one of you give the green light on this?

@bkueng bkueng merged commit c769fc7 into PX4:main Dec 13, 2023
86 of 87 checks passed
@niklaut niklaut deleted the feature/emcon branch December 13, 2023 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bkueng bkueng bkueng approved these changes

@sfuhrer sfuhrer Awaiting requested review from sfuhrer

@MaEtUgR MaEtUgR Awaiting requested review from MaEtUgR

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@niklaut @sfuhrer @bkueng @MaEtUgR