Adding rpm freq osd element and new format for rpm. #1315

dronejunkie · 2019-03-08T13:07:18Z

This PR is in response to this PR betaflight/betaflight#7671

dronejunkie · 2019-03-08T13:15:26Z

Here is the screen recording https://www.youtube.com/watch?v=ZxatONuCn_8

dronejunkie · 2019-03-09T02:25:53Z

Sorry about all of those commit points. Please let me know if you want me to squash them.

mikeller · 2019-03-09T03:43:24Z

src/js/tabs/osd.js

-                                    ctx.drawImage(img, (element.x + offsetX) * 12, (element.y + offsetY) * 18);
-                                }
+				//Add string to the preview.
+	                        if (element.constructor == String) { 


Not sure if adding this is actually the right thing to do - this adds complexity, and the same effect can be achieved easily by returning an array of objects, like it is done in

betaflight-configurator/src/js/tabs/osd.js

Lines 474 to 509 in 4dcc910

ARTIFICIAL_HORIZON: {

name: 'ARTIFICIAL_HORIZON',

desc: 'osdDescElementArtificialHorizon',

default_position: function () {

var position = 74;

if (OSD.constants.VIDEO_TYPES[OSD.data.video_system] != 'NTSC') {

position += FONT.constants.SIZES.LINE;

}

return position;

},

draw_order: 10,

positionable: function () {

return semver.gte(CONFIG.apiVersion, "1.39.0") ? true : false;

},

preview: function () {

var artificialHorizon = new Array();

for (var j = 1; j < 8; j++) {

for (var i = -4; i <= 4; i++) {

var element;

// Blank char to mark the size of the element

if (j != 4) {

element = { x: i, y: j, sym: SYM.BLANK };

// Sample of horizon

} else {

element = { x: i, y: j, sym: SYM.AH_BAR9_0 + 4 };

}

artificialHorizon.push(element);

}

}

return artificialHorizon;

}

},

.

Also, this does not check to make sure the element can fit onto the screen.

This was what I initially looked at but this only does one char per element as far I can see. I have thought of iterating through each strings and create individual element for each character. However this becomes very messy. I am new to this so I may have missed something.

RE making sure the element can fit, are you referring to this code below which re-position the element when it is dropped onto the canvas? This can be expanded.

if (display_item.preview.constructor !== Array) { // Standard preview, string type var overflows_line = FONT.constants.SIZES.LINE - ((position % FONT.constants.SIZES.LINE) + display_item.preview.length); if (overflows_line < 0) { position += overflows_line; } } else { // Advanced preview, array type var arrayElements = display_item.preview; var limits = OSD.searchLimitsElement(arrayElements); var selectedPositionX = position % FONT.constants.SIZES.LINE; var selectedPositionY = Math.trunc(position / FONT.constants.SIZES.LINE); if ((limits.minX < 0) && ((selectedPositionX + limits.minX) < 0)) { position += Math.abs(selectedPositionX + limits.minX); } else if ((limits.maxX > 0) && ((selectedPositionX + limits.maxX) >= FONT.constants.SIZES.LINE)) { position -= (selectedPositionX + limits.maxX + 1) - FONT.constants.SIZES.LINE; } if ((limits.minY < 0) && ((selectedPositionY + limits.minY) < 0)) { position += Math.abs(selectedPositionY + limits.minY) * FONT.constants.SIZES.LINE; } else if ((limits.maxY > 0) && ((selectedPositionY + limits.maxY) >= OSD.data.display_size.y)) { position -= (selectedPositionY + limits.maxY - OSD.data.display_size.y + 1) * FONT.constants.SIZES.LINE; } } }

Good point, I thought the array of objects could be used for strings as well. If not, extending that, and allowing the strings to be anchored to an arbitrary point makes more sense in my opinion.
And yes - we want to prevent users from placing elements off screen.

dronejunkie · 2019-03-11T13:28:59Z

Here is the screen video capture https://youtu.be/MSsPLAxVrf0. Placement of the element is pretty precise and user can not drag it outside of the canvas.

However you can not drag element to the edge of the bottom or left of the screen. This is because of the current existing logic below which re-align the x and y position when there is overflow. I am not sure why there is a +1 there, which become -1 i.e Y is one less and so is X. In affect you are losing one line.

position -= (selectedPositionX + limits.maxX + 1) - FONT.constants.SIZES.LINE;    (line 1751)
position -= (selectedPositionY + limits.maxY - OSD.data.display_size.y + 1) * FONT.constants.SIZES.LINE; (line 1756)

Here is a demonstration of what I meant https://youtu.be/TvEreM_Ofrw

I can have a separate formulae for my case if that is ok with you, which allow user to drag it all the way down to the edge. Here is what it should look like https://youtu.be/pcGGyogsLEg

mikeller · 2019-03-12T10:08:11Z

src/js/tabs/osd.js

@@ -803,7 +803,7 @@ OSD.constants = {
            default_position: -1,
            draw_order: 490,
            positionable: true,
-            preview: '226000'
+            preview: [ "22600","22600","22600","22600"]


Please use space after comma.

mikeller · 2019-03-12T10:08:38Z

src/js/tabs/osd.js

-        limits.maxY = Math.max(valor.y, limits.maxY);
-    });
+    if ((arrayElements.length > 0) && (arrayElements[0].constructor == String)) {
+	limits.maxY=arrayElements.length;


Indentation.

Also, please use space around operators.

mikeller · 2019-03-12T10:09:28Z

src/js/tabs/osd.js

@@ -1669,7 +1686,7 @@ OSD.GUI.preview = {
        var offsetX = 6;
        var offsetY = 9;

-        if (display_item.preview.constructor === Array) {
+        if ((display_item.preview.constructor === Array) && (display_item.preview[0].constructor != String)) {


This should use non-coercing inequality.

mikeller · 2019-03-12T10:11:04Z

src/js/tabs/osd.js

-                                    ctx.drawImage(img, (element.x + offsetX) * 12, (element.y + offsetY) * 18);
-                                }
+				//Add string to the preview.
+	                        if (element.constructor == String) { 


Indentation.

dronejunkie · 2019-03-13T19:44:42Z

I have done the requested changes. Please review.

mikeller · 2019-03-14T09:44:30Z

src/js/tabs/osd.js

-        limits.maxY = Math.max(valor.y, limits.maxY);
-    });
+    if ((arrayElements.length > 0) && (arrayElements[0].constructor === String)) {
+	limits.maxY = arrayElements.length;


I think you should check the indentation settings in the editor that you are using. Betaflight coding style states 4 spaces for indentation, and there are still plenty of tabs in your changes: https://github.com/betaflight/betaflight/blob/master/docs/development/CodingStyle.md#indentation

mikeller · 2019-03-14T09:44:56Z

locales/en/messages.json

@@ -3696,6 +3696,9 @@
    "osdDescElementEscRpm": {
        "message": "RPM reported by ESC telemetry"
    },
+    "osdDescElementEscRpmFreq": {
+        "message": "RPM Freq reported by ESC telemetry"


Can we change this to 'frequency' please?

dronejunkie · 2019-03-14T11:37:03Z

Sorry about the tab, I have been editing from a Windows laptop instead of my normal Linux Dev pc. It should be ok now. I have aligned the new element layout with the single char layout. As a result I could reuse existing logic for single char and do not need to distinct between array of string or single character. All tested fine.

mikeller · 2019-03-14T11:51:58Z

All good, the formatting looks good now. Unfortunately, with your changes pre-existing elements using a custom draw function can now moved around and positioned so that they wrap around the screen:

(This isn't possible before your change.)

dronejunkie · 2019-03-14T12:42:29Z

I could replicate this issue with build #131 Here is the video, skip to near the end
https://youtu.be/eylOIegBaU0

dronejunkie · 2019-03-15T06:03:34Z

I found a silly mistake in my code. This will definitely can cause the wrapping for the array of characters osd element. This however does not explain why I could replicate this issue in build #131. I will double check my testing of #131 again.

betaflight-configurator/src/js/tabs/osd.js

Lines 1125 to 1134 in 4592324

    
           if (arrayElements.length > 0) { 
        
               limits.maxY = arrayElements.length; 
        
               limits.minY = 0; 
        
               limits.minX = 0; 
        
               arrayElements.forEach(function(valor, indice, array) { 
        
                   limits.maxX = Math.max(valor.length, limits.maxX); 
        
               }); 
        
               limits.maxX--; 
        
               limits.maxY--; 
        
           } else {

I will do the change when I get home tonight. The above should only be applicable to the array of strings.

mikeller · 2019-03-15T06:06:50Z

Thanks for looking into this again @dronejunkie. It's well possible that the existing code is buggy as well, allowing this to happen some times, but your changes seemed to allow it to happen every time (or else I got 'lucky' 2 times out of 2 that I tried to replicate it with your changes).

dronejunkie · 2019-03-16T00:49:38Z

I tried with latest build and it is very easy to replicate this issue. You just need to place the cursor towards the top left corner and it will wrap every single time. Here is the video
https://youtu.be/cC8YI7eUaM4

dronejunkie · 2019-03-16T01:03:49Z

Latest fix should fix the wrapping issue for array of strings. The logic for detection/adjustment of wrapping for array of single char and array of strings are different. The array of single char one stay untouched. The testing I have done so far for the array of strings did not show any wrapping issue.

mikeller · 2019-03-16T08:00:27Z

Found the culprit that broke wrapping for arrays of char: #1165.

mikeller · 2019-03-16T08:05:43Z

Can you rebase and squash into one commit please?

…type, which is an array of string.

dronejunkie · 2019-03-16T12:33:06Z

rebased and squashed to one commit. Thanks for your help!

mikeller · 2019-03-16T21:23:36Z

Thanks for sticking with me - that regression on wraparound is nasty.

dronejunkie · 2019-03-17T08:18:51Z

@mikeller , I have a fix for it. Here is the test I have done.
https://youtu.be/ZnzQeR34jok

mikeller · 2019-03-17T08:33:20Z

@dronejunkie: Looks good! Thanks for looking into this!

dronejunkie · 2019-03-17T10:00:30Z

Do I raise a separate PR?

McGiverGim · 2019-03-17T10:09:28Z

Yes, this has been merged. You need a new PR.

dronejunkie mentioned this pull request Mar 8, 2019

Add OSD anti gravity element #1092

Merged

mikeller reviewed Mar 9, 2019

View reviewed changes

mikeller requested changes Mar 12, 2019

View reviewed changes

mikeller requested changes Mar 14, 2019

View reviewed changes

mikeller added this to the 10.5.0 milestone Mar 14, 2019

mikeller previously approved these changes Mar 16, 2019

View reviewed changes

mikeller added the Rebase Required label Mar 16, 2019

Added rpm freq osd element and new rpm osd layout. Added new element …

b390a48

…type, which is an array of string.

dronejunkie dismissed mikeller’s stale review via b390a48 March 16, 2019 12:31

dronejunkie force-pushed the rpm-osd branch from 06c2864 to b390a48 Compare March 16, 2019 12:31

mikeller approved these changes Mar 16, 2019

View reviewed changes

mikeller removed the Rebase Required label Mar 16, 2019

mikeller merged commit 9e3f279 into betaflight:master Mar 16, 2019

dronejunkie deleted the rpm-osd branch March 17, 2019 08:19

mikeller added the RN: UI label Apr 9, 2019

	ARTIFICIAL_HORIZON: {
	name: 'ARTIFICIAL_HORIZON',
	desc: 'osdDescElementArtificialHorizon',
	default_position: function () {
	var position = 74;
	if (OSD.constants.VIDEO_TYPES[OSD.data.video_system] != 'NTSC') {
	position += FONT.constants.SIZES.LINE;
	}
	return position;
	},
	draw_order: 10,
	positionable: function () {
	return semver.gte(CONFIG.apiVersion, "1.39.0") ? true : false;
	},
	preview: function () {
	var artificialHorizon = new Array();

	for (var j = 1; j < 8; j++) {
	for (var i = -4; i <= 4; i++) {

	var element;

	// Blank char to mark the size of the element
	if (j != 4) {
	element = { x: i, y: j, sym: SYM.BLANK };

	// Sample of horizon
	} else {
	element = { x: i, y: j, sym: SYM.AH_BAR9_0 + 4 };
	}
	artificialHorizon.push(element);
	}
	}
	return artificialHorizon;
	}
	},

Uh oh!

Adding rpm freq osd element and new format for rpm. #1315

Adding rpm freq osd element and new format for rpm. #1315

Uh oh!

Conversation

dronejunkie commented Mar 8, 2019

Uh oh!

dronejunkie commented Mar 8, 2019

Uh oh!

dronejunkie commented Mar 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dronejunkie Mar 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dronejunkie commented Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dronejunkie commented Mar 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dronejunkie commented Mar 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikeller commented Mar 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dronejunkie commented Mar 14, 2019

Uh oh!

dronejunkie commented Mar 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikeller commented Mar 15, 2019

Uh oh!

dronejunkie commented Mar 16, 2019

Uh oh!

dronejunkie commented Mar 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikeller commented Mar 16, 2019

Uh oh!

mikeller commented Mar 16, 2019

Uh oh!

dronejunkie commented Mar 16, 2019

Uh oh!

mikeller commented Mar 16, 2019

Uh oh!

dronejunkie commented Mar 17, 2019

Uh oh!

mikeller commented Mar 17, 2019

Uh oh!

dronejunkie commented Mar 17, 2019

Uh oh!

McGiverGim commented Mar 17, 2019

Uh oh!

Uh oh!

dronejunkie Mar 9, 2019 •

edited

Loading

dronejunkie commented Mar 11, 2019 •

edited

Loading

dronejunkie commented Mar 14, 2019 •

edited

Loading

mikeller commented Mar 14, 2019 •

edited

Loading

dronejunkie commented Mar 15, 2019 •

edited

Loading

dronejunkie commented Mar 16, 2019 •

edited

Loading