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

Use native build instead of x86/jetson target #31

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lida2003
Copy link
Contributor

@lida2003
Copy link
Contributor Author

sudo apt-get update
sudo apt-get upgrade

Untitled

Discussion: [ Ignore msposd_jetson for gs on jetson orin OpenIPC#29 ](OpenIPC#29)
@henkwiedig
Copy link
Collaborator

I like this more !

  • Can you change .github/workflows/build.yml as well to match this ?

@lida2003
Copy link
Contributor Author

@henkwiedig Please review (I just do it first time, consulting https://docs.github.com/en/actions), I don't know how to trigger the test. But those script and binary name should be simple for review.

And PLEASE send me a link if there is any steps for local test.

@henkwiedig
Copy link
Collaborator

Testbuild looks fine, hi3536dv100_fpv doesn't build locally as well with main. So no regression for that.
When you want to locally run msposd on x86 you will need receiving hardware, setup the msp stream to receive from the drone. Just like on jetson. Then use gstreamer to start the video and overlay it with msposd. The screenshot you postet is what you will get when msposd is runnig and no mspdata is received. So this is expected with no forther config.

What about the DISPLAY envvar ? I now testest more then 3 ways to set this on remote ssh session.
ssh ... 'export DISPLAY=:0; msposd ...
ssh -o SendEnv=DISPLAY root@.... msposd....
edit .bashrc
The way it is now we completely disable other users to set the DISPLAY as they wish.
Unless there is really good reason for this i think this should be removed.

@lida2003
Copy link
Contributor Author

lida2003 commented Nov 26, 2024

If you don't use it, then a segfault when launching from ssh, and no warning, no feedback.

@henkwiedig
Copy link
Collaborator

Yes, but why setting this in the application, especially forcefully overwrite it ?
You can use one of the million other way to set the DISLPLAY ?
There might be setups with two displays, one for Video one for telemetry/mission plan.
The user will not have the option to choose which is which.

@lida2003
Copy link
Contributor Author

Yes, but why setting this in the application, especially forcefully overwrite it ?

Not forcefully.

You can use one of the million other way to set the DISLPLAY ? There might be setups with two displays, one for Video one for telemetry/mission plan. The user will not have the option to choose which is which.

That's what I consider.

  • As I have said if launch from ssh, it will segfault.
  • If you have env set, then posix api did as user set.
  • If you haven't set, posix api will ask, and set default to current active display.

Then to provide code's robustness of NOT properly setting env, casuing segfault issue.

@henkwiedig
Copy link
Collaborator

setenv("DISPLAY", ":0", 0); does what you want, without the if (getenv("DISPLAY") == NULL) ...

@lida2003
Copy link
Contributor Author

lida2003 commented Nov 27, 2024

setenv("DISPLAY", ":0", 0); does what you want, without the if (getenv("DISPLAY") == NULL) ...

This is what we mean: if we don't have this env, it will segfault. If there is no display set, then we simply use default display.
It's a straight forward way to do the default setting. Of course, user can set other values, then there will NOT be NULL.

if (getenv("DISPLAY") == NULL) {
/*
* Use default display screen, especially launch from console
* ToDo: x86 linux should implement this code also, test needed.
*/
setenv("DISPLAY", ":0", 1);
}

@henkwiedig
Copy link
Collaborator

henkwiedig commented Nov 27, 2024

this

setenv("DISPLAY", ":0", 0); 

is equivalant to this:

         if (getenv("DISPLAY") == NULL) { 
             /* 
              * Use default display screen, especially launch from console 
              * ToDo: x86 linux should implement this code also, test needed. 
              */ 
             setenv("DISPLAY", ":0", 1); 
         } 

hence the "0". from man 3 setenv

.... If name does exist in the environment, then its value is changed to value if overwrite is nonzero; if overwrite is zero, then the value of name is not changed ....

Edit: we can remove the ifdef in my opinion

@lida2003
Copy link
Contributor Author

lida2003 commented Nov 27, 2024

You can do the test. If you don't have this code. It'll be a segfault launching from ssh.

EDIT: Or did you mean use setenv("DISPLAY", ":0", 0); ?

@henkwiedig
Copy link
Collaborator

henkwiedig commented Nov 27, 2024

Yes just use the non overwrite option from setenv. "0" as thid argument. Then the whole "if" part is useless.
We can also remove the ifdef.

All boils down to one line ! More readable ;)

@lida2003
Copy link
Contributor Author

@henkwiedig It doesn't work on jetson.

Can you check if the below is what we meant to do.

If I didn't mis-understand, please let me know about setenv

I think lanuching from remote ssh(putty) mgiht be different or something about the understanding of the posix API.

daniel@daniel-nvidia:~/Work/msposd/release/jetson$ echo $DISPLAY

daniel@daniel-nvidia:~/Work/msposd/release/jetson$ ./msposd --master 127.0.0.1:14560  --osd -r 50 --ahi 1 --matrix 11 -v
Ver: 8a82c65-dirty Compiled at: 20241127_153132
Listen on port :127.0.0.1:14560
MSP to OSD mode!Verbose mode!Majestic width:1920,height:1080
Cannot open display
Loading /home/daniel/Work/msposd/release/jetson/font.png for 1000p mode
Font file res 36:13824 pages:1
Loading small size glyphs /home/daniel/Work/msposd/release/jetson/font_hd.png for 1000p mode
Glyph size:36:54 on a 53:20 matrix. Overlay 1912:1080
Create_region PixelFormat:11 Size: 1912:1080 X_Offset:8 Y_Offset:0 results: -1
Segmentation fault (core dumped)
daniel@daniel-nvidia:~/Work/msposd/release/jetson$ git diff
diff --git a/osd/util/Render_x86.c b/osd/util/Render_x86.c
index a49942b..9f42317 100644
--- a/osd/util/Render_x86.c
+++ b/osd/util/Render_x86.c
@@ -72,20 +72,8 @@ int Init_x86(uint16_t *width, uint16_t *height) {
             forcefullscreen=false;
 #ifdef _DEBUG_x86
         forcefullscreen=false;
+        setenv("DISPLAY", ":0", 0);
 #endif
-
-
-#ifdef _x86
-        if (getenv("DISPLAY") == NULL) {
-            /*
-             * Use default display screen, especially launch from console
-             * ToDo: x86 linux should implement this code also, test needed.
-             */
-            setenv("DISPLAY", ":0", 1);
-        }
-#endif
-
-

         display = XOpenDisplay(NULL);
         if (!display) {

@henkwiedig
Copy link
Collaborator

I assume _DEBUG_x86 was set durting compile time ?

@lida2003
Copy link
Contributor Author

lida2003 commented Nov 27, 2024

_DEBUG_x86 was set durting compile time ?

YES

EDIT: check PR-diff

图片

@lida2003
Copy link
Contributor Author

@henkwiedig
Ah..... it's _DEBUG_x86 not _x86

what this MACRO for?

@lida2003
Copy link
Contributor Author

lida2003 commented Nov 27, 2024

@henkwiedig I'll left _DEBUG_x86 as it is. Just optimize setenv for native build.

BTW, it works on jetson, should work on x86 either, as posix api.

EDIT: add _DEBUG_x86 MACRO optimization & clarification #33 for _DEBUG_x86 clarification.

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

Successfully merging this pull request may close these issues.

2 participants