Skip to content

Fix minor issues #2287

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
Merged
merged 1 commit into from
Nov 23, 2020
Merged

Fix minor issues #2287

merged 1 commit into from
Nov 23, 2020

Conversation

@haslinghuis
Copy link
Member

@haslinghuis haslinghuis commented Nov 22, 2020 •
edited
Loading

Minor issues found and fixed:

  • connectionType not established before connecting.
  • Removed unused variable offset
  • Moved logging of crc errors out of msp.js to better show where it's coming from
  • Intermittent undefined code in callback after save and reboot
Error in event handler: TypeError: Cannot read property 'code' of undefined
    at MspHelper.process_data (chrome-extension://ajdoecjhplhoahggfmgflfnlechcljoc/js/msp/MSPHelper.js:1624:38)
    at chrome-extension://ajdoecjhplhoahggfmgflfnlechcljoc/js/msp.js:226:13
    at Array.forEach (<anonymous>)
    at Object.notify (chrome-extension://ajdoecjhplhoahggfmgflfnlechcljoc/js/msp.js:225:24)
    at Object._dispatch_message (chrome-extension://ajdoecjhplhoahggfmgflfnlechcljoc/js/msp.js:220:14)
    at Object.read (chrome-extension://ajdoecjhplhoahggfmgflfnlechcljoc/js/msp.js:182:22)
    at read_serial (chrome-extension://ajdoecjhplhoahggfmgflfnlechcljoc/js/serial_backend.js:562:13)
  • Intermittent beginning to see Reboot request accepted by changing timeout from 100 to 200 in serial_backend.js
MSPHelper.js:632 Settings Saved in EEPROM
MSPHelper.js:723 Reboot request accepted
serial.js:404 serial: receive error: UNDEFINED: device_lost
serial.js:217 serial: closed connection with ID: 4, Sent: 7077 bytes, Received: 79082 bytes

mikeller
mikeller reviewed Nov 22, 2020
View reviewed changes
Copy link
Member

@mikeller mikeller left a comment

Choose a reason for hiding this comment

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

Your rummaging around to fix the TCP connection is finding some valuable fixes. Thanks!

src/js/msp/MSPHelper.js Outdated
if (callback) callback({'command': code, 'data': data, 'length': data.byteLength, 'crcError': crcError});
for (let i = dataHandler.callbacks.length - 1; i >= 0; i--) { // iterating in reverse because we use .splice which modifies array length
if (dataHandler.callbacks[i] !== undefined) {
if (dataHandler.callbacks[i].code === code) {
Copy link
Member

Choose a reason for hiding this comment

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

More concise to go if (dataHandler.callbacks[i]?.code === code) {.

Copy link
Member Author

Choose a reason for hiding this comment

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

For reference: https://github.com/claudepache/es-optional-chaining

@mikeller mikeller added this to the 10.8.0 milestone Nov 22, 2020
@haslinghuis
Copy link
Member Author

It's recommended to replace all var variables to let or const.
I can do it here or in a separate PR per file.

mikeller
mikeller reviewed Nov 22, 2020
View reviewed changes
src/js/msp/MSPHelper.js Outdated
if (callback) callback({'command': code, 'data': data, 'length': data.byteLength, 'crcError': crcError});
for (let i = dataHandler.callbacks.length - 1; i >= 0; i--) { // iterating in reverse because we use .splice which modifies array length
if (dataHandler.callbacks[i] !== undefined) {
if (dataHandler.callbacks[i]?.code === code) {
Copy link
Member

Choose a reason for hiding this comment

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

In this case (?.) the outer if (dataHandler.callbacks[i] !== undefined) { is not needed anymore, as dataHandler.callbacks[i]?.code will yield undefined if dataHandler.callbacks[i] does not exist. :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Err, one Control-Z to many. Fixed

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@mikeller mikeller merged commit aed1408 into betaflight:master Nov 23, 2020
@haslinghuis haslinghuis deleted the msp_undefined branch November 23, 2020 22:21
@mikeller mikeller modified the milestones: 10.8.0, 10.7.1 Oct 10, 2021
mikeller added a commit that referenced this pull request Oct 10, 2021
@mikeller
Merge pull request #2287 from haslinghuis/msp_undefined
985daf3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mikeller mikeller mikeller approved these changes

Assignees

No one assigned

Labels

RN: BUGFIX

Projects

None yet

Milestone

10.7.1

Development

Successfully merging this pull request may close these issues.

2 participants

@haslinghuis @mikeller