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.

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

Added new DSHOT debug modes #3262

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

damosvil
Copy link

@damosvil damosvil commented Jan 21, 2023

This pr matches functionality in betaflight/betaflight#12170 and betaflight/betaflight#11694

Added new DSHOT EDT modes to blackbox tab. As their names are longer than usual I made the drop down list controls wider.
Also added FAILSAFE mode I found in blackbox explorer.

It can be used with Bluejay 0.20.1 RC2 or above.


Depends on https://github.com/betaflight/betaflight/pull/12170

@McGiverGim
Copy link
Member

Something is wrong, you are modifying a lot of files, including different languages files.

@damosvil
Copy link
Author

damosvil commented Jan 21, 2023

@McGiverGim This pr adds:

This is a work in progress/draft pr. Other functionality may be added over time.

@damosvil
Copy link
Author

damosvil commented Jan 21, 2023

@McGiverGim I am new at adding new features to Configurator, so I may be breaking some rules I don't know.
Please, could you let me know why is this wrong?

@McGiverGim
Copy link
Member

You don't must change the locales files, only the English one. The translations are made at Crowdin and will update the others languages automatically.

@damosvil
Copy link
Author

@McGiverGim Ok, I will restore them

@github-actions

This comment has been minimized.

@damosvil damosvil force-pushed the edt_events branch 2 times, most recently from 7b5e4cf to cc01eec Compare January 22, 2023 21:06
@sonarcloud
Copy link

sonarcloud bot commented Jan 22, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@@ -1,6 +1,6 @@
import semver from "semver";
import FC from "../fc";
import { API_VERSION_1_45 } from "../data_storage";
import { API_VERSION_1_46 } from "../data_storage";
Copy link
Member

Choose a reason for hiding this comment

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

❤️

Copy link
Member

Choose a reason for hiding this comment

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

Usually the bump version is done by ours, but this is a draft so I think is needed to see how the code looks. When final version of Configurator is released, we will bump the version and a rebase will fix the issue.

Copy link
Member

Choose a reason for hiding this comment

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

I just like ppl using modules 🤓

Copy link
Author

@damosvil damosvil Mar 18, 2023

Choose a reason for hiding this comment

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

I rolled back this change, because the import was not used in this module

Comment on lines 278 to 275
escprotocol: FC.PID_ADVANCED_CONFIG.fast_pwm_protocol + 1,
feature3: FC.FEATURE_CONFIG.features.isEnabled('MOTOR_STOP'),
feature4: FC.FEATURE_CONFIG.features.isEnabled('MOTOR_STOP'),
feature9: FC.FEATURE_CONFIG.features.isEnabled('3D'),
feature20: FC.FEATURE_CONFIG.features.isEnabled('ESC_SENSOR'),
feature23: FC.FEATURE_CONFIG.features.isEnabled('ESC_SENSOR'),
Copy link
Member

Choose a reason for hiding this comment

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

This change in the number of feature is because we have some bug in the actual code?

Copy link
Author

@damosvil damosvil Jan 24, 2023

Choose a reason for hiding this comment

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

Yes, feature 3 corresponds to previous feature 0, and feature 23 corresponds to old feature 20

Copy link
Member

Choose a reason for hiding this comment

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

@McGiverGim should I make a separate bugfix for this?

Copy link
Member

Choose a reason for hiding this comment

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

With all the changes done in firmware/configurator I'm not too sure how affects this, but if it is wrong I suppose than yes.

Copy link
Member

Choose a reason for hiding this comment

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

This change should not be made looking at the table:

feature bit group name
0 0 rxMode RX_PPM
1 2 other INFLIGHT_ACC_CALL
2 3 rxMode RX_SERIAL
3 4 escMotorStop MOTOR_STOP
4 5 other SERVO_TILT
5 6 other SOFTSERIAL
6 7 gps GPS
7 9 other SONAR
8 10 telemetry TELEMETRY
9 12 3D 3D
10 13 rxMode RX_PARALLEL_PWM
11 14 rxMode RX_MSP
12 15 rssi RSSI_ADC
13 16 other LED_STRIP
14 17 other DISPLAY
15 18 other OSD
16 20 other CHANNEL_FORWARDING
17 21 other TRANSPONDER
18 22 other AIRMODE
19 25 rxMode RX_SPI
20 27 escSensor ESC_SENSOR
21 28 antiGravity ANTI_GRAVITY

https://github.com/betaflight/betaflight/blob/b1433123c56598ce8ea14357d05adc9e6e1c28ca/src/main/config/feature.h#L32-L57

typedef enum {
FEATURE_RX_PPM = 1 << 0,
FEATURE_INFLIGHT_ACC_CAL = 1 << 2,
FEATURE_RX_SERIAL = 1 << 3,
FEATURE_MOTOR_STOP = 1 << 4,
FEATURE_SERVO_TILT = 1 << 5,
FEATURE_SOFTSERIAL = 1 << 6,
FEATURE_GPS = 1 << 7,
FEATURE_RANGEFINDER = 1 << 9,
FEATURE_TELEMETRY = 1 << 10,
FEATURE_3D = 1 << 12,
FEATURE_RX_PARALLEL_PWM = 1 << 13,
FEATURE_RX_MSP = 1 << 14,
FEATURE_RSSI_ADC = 1 << 15,
FEATURE_LED_STRIP = 1 << 16,
FEATURE_DASHBOARD = 1 << 17,
FEATURE_OSD = 1 << 18,
FEATURE_CHANNEL_FORWARDING = 1 << 20,
FEATURE_TRANSPONDER = 1 << 21,
FEATURE_AIRMODE = 1 << 22,
FEATURE_RX_SPI = 1 << 25,
//FEATURE_SOFTSPI = 1 << 26, (removed)
FEATURE_ESC_SENSOR = 1 << 27,
FEATURE_ANTI_GRAVITY = 1 << 28,
//FEATURE_DYNAMIC_FILTER = 1 << 29, (removed)
} features_e;

Copy link
Member

Choose a reason for hiding this comment

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

🤔 This should be 27

Copy link
Author

Choose a reason for hiding this comment

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

updated according to #3382

@damosvil
Copy link
Author

@McGiverGim @haslinghuis rebased to master, ready for review

@damosvil damosvil requested review from McGiverGim, chmelevskij and haslinghuis and removed request for McGiverGim, chmelevskij and haslinghuis March 17, 2023 23:46

This comment has been minimized.

Comment on lines 816 to 820
if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_46)) {
$('.checkboxDshotEdt').toggle(protocolConfigured && digitalProtocol && dshotBidirElement.is(':checked') && !$("input[name='ESC_SENSOR']").is(':checked'));
} else {
$('.checkboxDshotEdt').toggle(false);
}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just

Suggested change
if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_46)) {
$('.checkboxDshotEdt').toggle(protocolConfigured && digitalProtocol && dshotBidirElement.is(':checked') && !$("input[name='ESC_SENSOR']").is(':checked'));
} else {
$('.checkboxDshotEdt').toggle(false);
}
$('.checkboxDshotEdt').toggle(protocolConfigured && digitalProtocol && dshotBidirElement.is(':checked') && !$("input[name='ESC_SENSOR']").is(':checked') && semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_46));

Copy link
Author

Choose a reason for hiding this comment

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

refactored

@ctzsnooze
Copy link
Member

ctzsnooze commented Dec 4, 2023

Thanks for keeping active here. I look forward to testing this with a suitable bluejay version.
Would it be possible to include a link in the opening comment to a suitable ESC firmware to test with?

This comment has been minimized.

@haslinghuis haslinghuis modified the milestones: 10.10.0, 11.0 Dec 5, 2023
Added checkbox to activate edt

Restore other translation files than the english one

Added logic to handle DSHOT edt checkbox in motors tab.
Bumped API_VERSION to 1.46.
Added dshot_edt setting to MSP_MOTOR_CONFIG frame.
Added dshot_edt setting to MSP_SET_MOTOR_CONFIG frame.
Updated API_VERSION in generate_filename.

Fixed features in motors tab.
Added exclusive selection between ESC_SENSOR and EDT options.

Fixed a bug

Fixed review findings

Removed unecessary imports

Fixed review findings

Fixed review findings

Added debug mode to match edt_events Betaflight branch

Fixed RPM_LIMIT debug mode name

Small refactoring to be coherent to current code style
Copy link

sonarcloud bot commented Dec 13, 2023

Quality Gate Failed Quality Gate failed

Failed conditions

4.3% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

Copy link
Contributor

Do you want to test this code? Here you have an automated build:
Betaflight-Configurator-Android
Betaflight-Configurator-Linux
Betaflight-Configurator-macOS
Betaflight-Configurator-Windows
WARNING: It may be unstable and result in corrupted configurations or data loss. Use only for testing!

Comment on lines +755 to +764
const dshotEdtElement = $('input[id="dshotEdt"]');

dshotEdtElement.prop('checked', FC.MOTOR_CONFIG.use_dshot_edt).trigger("change");
dshotEdtElement.on("change", function () {
const value = dshotEdtElement.is(':checked');
const newValue = (value !== FC.MOTOR_CONFIG.use_dshot_edt) ? 'On' : 'Off';

self.analyticsChanges['DshotEdt'] = newValue;
FC.MOTOR_CONFIG.use_dshot_edt = value;
});
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about the code style here.

Better to use this or similar way to get the info about the actual order, sometimes things get weird with closures in js.

Also, not sure what triggering change event and then adding on handler does here 🤔

Something like the following is a bit more streamlined/usual in js land

Suggested change
const dshotEdtElement = $('input[id="dshotEdt"]');
dshotEdtElement.prop('checked', FC.MOTOR_CONFIG.use_dshot_edt).trigger("change");
dshotEdtElement.on("change", function () {
const value = dshotEdtElement.is(':checked');
const newValue = (value !== FC.MOTOR_CONFIG.use_dshot_edt) ? 'On' : 'Off';
self.analyticsChanges['DshotEdt'] = newValue;
FC.MOTOR_CONFIG.use_dshot_edt = value;
});
const dshotEditCheckbox = $('#dshotEdt');
dshotEditCheckbox.prop('checked', FC.MOTOR_CONFIG.use_dshot_edt)
.on("change", function () {
const isChecked = $(this).is(':checked');
const newValue = isChecked !== FC.MOTOR_CONFIG.use_dshot_edt ? 'On' : 'Off';
self.analyticsChanges['DshotEdt'] = newValue;
FC.MOTOR_CONFIG.use_dshot_edt = isChecked;
})
.trigger("change");

Comment on lines +94 to +96
<div style="float: left; height: 20px; margin-right: 14px; margin-left: 3px;" class="checkboxDshotEdt">
<input type="checkbox" id="dshotEdt" class="toggle" />
<span class="freelabel" id="dshotEdtLabel" i18n="configurationDshotEdt"></span>
Copy link
Member

Choose a reason for hiding this comment

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

Ooof, these floats look painful 🫠 But probably better for style consistency. More of a note to self/team 🙈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

8 participants