Skip to content

Conversation

@Ahmed-hadzi
Copy link

@Ahmed-hadzi Ahmed-hadzi commented Nov 1, 2025

User description

While I was working on an AI object tracking project, I realised that I can't see what is my AI tracking while I am flying with my goggles on, waiting to take control if something goes wrong. So, I've modified INAV firmware to accept dynamic position changing of Custom Elements in-flight, using the MSP command 167.

It takes 8 bits as item id, for example 147 for Custom Element 1, and then 16 bits of position data formatted in the INAV format (x & 0x3F) | ((y & 0x3F) << 6). I will also post the companion-computer side of the code below. I think this would be a great addition in the future release of INAV, as it does not affect the code that much, but it does enable this feature for many people who are missing it.

`import serial
import struct
import time

-----------------------------

CONFIG

-----------------------------

PORT = "/dev/serial0"
BAUD = 57600
MSP_OSD_CUSTOM_CHARACTER = 167 # firmware MSP command

-----------------------------

Connect to serial

-----------------------------

def connect_serial():
while True:
try:
ser = serial.Serial(PORT, BAUD, timeout=1)
time.sleep(0.2)
print(f"Connected to {PORT} at {BAUD} baud.")
return ser
except Exception as e:
print(f"Serial connection failed: {e}. Retrying...")
time.sleep(1)

-----------------------------

Send OSD position

-----------------------------

def osd_custom_pos(ser, item, x, y):
# Encode position (always visible → bit 13 set)
pos = ((x & 0x3F) | ((y & 0x3F) << 6) | (1 << 13))

# Build MSP payload: [item_id, pos_low, pos_high]
payload = struct.pack("<BH", item, pos)

# Build MSP frame
size = len(payload)
checksum = 0
frame = bytearray(b"$M<")
frame.append(size)
frame.append(MSP_OSD_CUSTOM_CHARACTER)
checksum ^= size
checksum ^= MSP_OSD_CUSTOM_CHARACTER

for b in payload:
    frame.append(b)
    checksum ^= b

frame.append(checksum)

# Send via serial
ser.write(frame)
ser.flush()

print(f"Sent OSD item {item} → ({x},{y})  pos={pos:#06x}")

-----------------------------

Main program

-----------------------------

def main():
ser = connect_serial()

while True:
    try:
        # Example usage
        osd_custom_pos(ser, 148, 10, 5)
        time.sleep(0.5)
        osd_custom_pos(ser, 148, 21, 6)
        time.sleep(0.5)
        osd_custom_pos(ser, 148, 28, 11)
        time.sleep(0.5)

    except KeyboardInterrupt:
        ser.close()
        print("Serial connection closed.")

if name == "main":
main()
`

This is an example of how this new MSP command can be used. It supports any of the Custom Elements, in any format or combination. I sincerely hope you will take this into consideration.


PR Type

Enhancement


Description

This description is generated by an AI tool. It may have inaccuracies

  • Adds support for dynamic in-flight position changing of Custom OSD Elements via MSP command 167

  • Enables companion computers to update Custom Element positions without rebooting the flight controller

  • Uses 8-bit item ID and 16-bit position data encoded in INAV format (x & 0x3F) | ((y & 0x3F) << 6)

  • Allows AI object tracking and other real-time OSD element repositioning use cases

  • Minimal code impact with significant new functionality for external systems


Diagram Walkthrough

flowchart LR
  CC["Companion Computer"] -- "MSP 167 command<br/>item_id + position" --> FC["Flight Controller"]
  FC -- "Update OSD<br/>Element Position" --> OSD["Custom OSD<br/>Element"]
Loading

File Walkthrough

Relevant files

@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Nov 1, 2025

PR Compliance Guide 🔍

All compliance sections have been disabled in the configurations.

@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Nov 1, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
PR is invalid due to build artifacts

The PR is invalid because it contains auto-generated build files from tools like
CMake. These files should be removed from the commit history, and the .gitignore
file should be updated to prevent them from being tracked.

Examples:

src/CMakeFiles/4.1.1/CompilerIdC/CMakeCCompilerId.c [1-934]
#ifdef __cplusplus
# error "A C++ compiler has been selected for C."
#endif

#if defined(__18CXX)
# define ID_VOID_MAIN
#endif
#if defined(__CLASSIC_C__)
/* cv-qualifiers did not exist in K&R C */
# define const

 ... (clipped 924 lines)
src/CMakeFiles/Makefile.cmake [1-4116]
# CMAKE generated file: DO NOT EDIT!
# Generated by "Unix Makefiles" Generator, CMake Version 4.1

# The generator used is:
set(CMAKE_DEPENDS_GENERATOR "Unix Makefiles")

# The top level Makefile was generated from the following files:
set(CMAKE_MAKEFILE_DEPENDS
  "CMakeCache.txt"
  "/Users/ahmed/Desktop/Projects/INAV-RPiOSD/inav/.git/HEAD"

 ... (clipped 4106 lines)

Solution Walkthrough:

Before:

// PR contains both source code and generated build artifacts.
// Git status (conceptual):
M src/main/fc/fc_msp.c
M src/main/io/osd.c
...
A src/CMakeFiles/4.1.1/CompilerIdC/CMakeCCompilerId.c
A src/CMakeFiles/Makefile.cmake
A src/src/main/target/AXISFLYINGF7PRO/.../DependInfo.cmake
... (many other generated files)

After:

// PR contains only the intended source code changes.
// Git status (conceptual):
M src/main/fc/fc_msp.c
M src/main/io/osd.c
M src/main/io/osd.h
M src/main/io/osd/custom_elements.c
M src/main/io/osd/custom_elements.h
M src/main/msp/msp_protocol.h

// .gitignore file is updated to exclude build artifacts
/src/CMakeFiles/
/src/src/main/target/**/CMakeFiles/
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical submission error; the inclusion of numerous build-related files makes the PR unreviewable and unmergable, which is a blocking issue.

High
Possible issue
Remove generated build file from repository
Suggestion Impact:The generated CMake DependInfo.cmake file content was effectively removed from the PR, replacing the 474-line file with a minimal stub, thereby removing user-specific absolute paths from the repo.

code diff:

@@ -1,474 +1 @@

Remove the generated CMake build file from the repository. It contains
user-specific absolute paths that will cause build failures for other developers
and should be added to .gitignore.

src/src/main/target/AXISFLYINGF7PRO/CMakeFiles/AXISFLYINGF7PRO_for_bl.elf.dir/DependInfo.cmake [1-474]

-# Consider dependencies only in project.
-set(CMAKE_DEPENDS_IN_PROJECT_ONLY OFF)
+# This file should be removed from the repository.
 
-# The set of languages for which implicit dependencies are needed:
-set(CMAKE_DEPENDS_LANGUAGES
-  "ASM"
-  )
-# The set of files for implicit dependencies of each language:
-set(CMAKE_DEPENDS_CHECK_ASM
-  "/Users/ahmed/Desktop/Projects/INAV-RPiOSD/inav/lib/main/CMSIS/DSP/Source/TransformFunctions/arm_bitreversal2.S" "/Users/ahmed/Desktop/Projects/INAV-RPiOSD/inav/src/src/main/target/AXISFLYINGF7PRO/CMakeFiles/AXISFLYINGF7PRO_for_bl.elf.dir/__/__/__/__/lib/main/CMSIS/DSP/Source/TransformFunctions/arm_bitreversal2.S.obj"
-  "/Users/ahmed/Desktop/Projects/INAV-RPiOSD/inav/src/main/startup/startup_stm32f722xx.s" "/Users/ahmed/Desktop/Projects/INAV-RPiOSD/inav/src/src/main/target/AXISFLYINGF7PRO/CMakeFiles/AXISFLYINGF7PRO_for_bl.elf.dir/__/__/startup/startup_stm32f722xx.s.obj"
-  )
-set(CMAKE_ASM_COMPILER_ID "GNU")
-
-# Preprocessor definitions for this target.
-set(CMAKE_TARGET_DEFINITIONS_ASM
-...
-
-# The include file search paths:
-set(CMAKE_ASM_TARGET_INCLUDE_PATH
-  "main/target/AXISFLYINGF7PRO"
-  "/Users/ahmed/Desktop/Projects/INAV-RPiOSD/inav/lib/main/STM32F7/Drivers/STM32F7xx_HAL_Driver/Inc"
-  "/Users/ahmed/Desktop/Projects/INAV-RPiOSD/inav/lib/main/STM32F7/Drivers/CMSIS/Device/ST/STM32F7xx/Include"
-...
-

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies that a generated build file containing user-specific absolute paths (/Users/ahmed/...) has been added, which will break the build for other developers and is bad practice for version control.

High
Add payload size validation check
Suggestion Impact:The commit added the proposed dataSize < 3 check and returns MSP_RESULT_ERROR before reading, implementing the safety validation. It also changed the layout index, but the size check matches the suggestion’s intent.

code diff:

+        if (dataSize < 3) {
++        return MSP_RESULT_ERROR;
++       }
         uint8_t item;
         sbufReadU8Safe(&item, src);

Add a payload size check in the MSP_OSD_CUSTOM_POSITION handler to ensure the
incoming data is at least 3 bytes before reading from the buffer.

src/main/fc/fc_msp.c [2718-2731]

 case MSP_OSD_CUSTOM_POSITION: {
+    if (dataSize < 3) {
+        return MSP_RESULT_ERROR;
+    }
     uint8_t item;
     sbufReadU8Safe(&item, src);
     if (item < OSD_ITEM_COUNT){ // item == addr
         osdEraseCustomItem(item);
         osdLayoutsConfigMutable()->item_pos[0][item] = sbufReadU16(src) | (1 << 13);
         osdDrawCustomItem(item);
     }
     else{
         return MSP_RESULT_ERROR;
     }
 
     break;
 }

[Suggestion processed]

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a missing payload size check, which could lead to a buffer over-read and undefined behavior. This is a significant robustness and security improvement.

Medium
Prevent use of uninitialized variable
Suggestion Impact:The commit added a default case with an early return in the switch, matching the suggested safeguard against invalid items.

code diff:

+        default:
++           return;
     }

In osdEraseCustomItem, add a default case to the switch statement that returns
from the function. This prevents unintended side effects when an item that is
not a custom element is passed.

src/main/io/osd.c [6593-6620]

 switch(item){
     case 147:
         customElementIndex = 0;
         break;
     case 148:
         customElementIndex = 1;
         break;
     case 149:
         customElementIndex = 2;
         break;
     case 154:
         customElementIndex = 3;
         break;
     case 155:
         customElementIndex = 4;
         break;
     case 156:
         customElementIndex = 5;
         break;
     case 157:
         customElementIndex = 6;
         break;
     case 158:
         customElementIndex = 7;
         break;
+    default:
+        return;
 }
 
 uint8_t len = customElementLength(customElementIndex);

[Suggestion processed]

Suggestion importance[1-10]: 7

__

Why: While the variable customElementIndex is initialized, the suggestion correctly identifies that an invalid item would cause the function to unintentionally modify the element at index 0, which is a logic bug.

Medium
General
Remove generated file from version control
Suggestion Impact:The commit removed the generated Makefile.cmake content (a full file deletion), aligning with the suggestion to remove this generated, environment-specific file from the repository.

code diff:

@@ -1,4116 +1 @@

Remove the generated Makefile.cmake file from version control. This file is
environment-specific and should be ignored by adding the CMakeFiles directory to
.gitignore.

src/CMakeFiles/Makefile.cmake [1-9]

-# CMAKE generated file: DO NOT EDIT!
-# Generated by "Unix Makefiles" Generator, CMake Version 4.1
+# This file should be removed from the pull request and repository.
 
-# The generator used is:
-set(CMAKE_DEPENDS_GENERATOR "Unix Makefiles")
-
-# The top level Makefile was generated from the following files:
-set(CMAKE_MAKEFILE_DEPENDS
-  "CMakeCache.txt"
-...
-

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies that a generated build file, which includes user-specific absolute paths, should not be committed to version control, as this is a critical repository health and build process issue.

High
Avoid using a hardcoded layout index
Suggestion Impact:The commit changed the assignment to use getCurrentLayout() instead of the hardcoded index 0, aligning with the suggestion to use the active layout.

code diff:

-            osdLayoutsConfigMutable()->item_pos[0][item] = sbufReadU16(src) | (1 << 13);
+            osdLayoutsConfigMutable()->item_pos[getCurrentLayout()][item] = sbufReadU16(src) | (1 << 13);
             osdDrawCustomItem(item);

Instead of hardcoding the OSD layout index to 0, consider using the currently
active layout or extending the MSP command to allow specifying the layout index.

src/main/fc/fc_msp.c [2723]

-osdLayoutsConfigMutable()->item_pos[0][item] = sbufReadU16(src) | (1 << 13);
+// Suggestion: use the current layout instead of a hardcoded one.
+// Note: `currentLayout` is a global variable defined in `osd.c` and might need to be made accessible here.
+extern uint8_t currentLayout;
+osdLayoutsConfigMutable()->item_pos[currentLayout][item] = sbufReadU16(src) | (1 << 13);

[Suggestion processed]

Suggestion importance[1-10]: 5

__

Why: The suggestion correctly points out the use of a hardcoded layout index 0, which limits the feature's flexibility. Proposing to use the current layout or a new MSP parameter is a valid design improvement.

Low
  • Update

Copy link
Collaborator

@stronnag stronnag left a comment

Choose a reason for hiding this comment

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

New MSP commands should be MSPv2 identifiers.
Please remove MSP_OSD_CUSTOM_POSITION from msp_protocol.h and define / add a new MSP2_OSD_CUSTOM_POSITION id to one of the MSP2 headers.

@MrD-RC
Copy link
Member

MrD-RC commented Nov 2, 2025

Surely the "custom" part can be dropped. Then have an MSPv2 call that can change the position of any OSD element. Just pass the ID of the element and the new position. For example MSP2_OSD_UPDATE_POSITION

@Ahmed-hadzi
Copy link
Author

Ahmed-hadzi commented Nov 2, 2025

I modified the code to remove build errors. Also, I've removed the MSP command from msp_protocol.h and added it as a MSPv2 command (MSP2_INAV_OSD_UPDATE_POSITION 0x2118). Tested it and it is still functioning as expected.

It is pretty much impossible to implement this for every OSD object as there is no way to accurately measure the length of the objects. Since there is no native function to remove an item from the screen without refreshing everything and causing flickering, my function needs to know the exact length of the OSD item, in order to replace it with blank characters. For custom elements, this is possible as there already is the variable "prevLength", but it does not exist for other OSD items. Also, some items are way too complex for this, like the artificial horizon. Nonetheless, they are called "Custom Elements" for a reason, so I believe this implementation is pretty reasonable.

Also worth mentioning, the MSPv2 approach invalidates my companion-side code I posted before.

@MrD-RC
Copy link
Member

MrD-RC commented Nov 2, 2025

There are some things that should never be moved. The AHI being one of them. Well, anything involving the HUD. For the standard elements, it should be fine. The only issue would be if you are constantly moving the element, which would cause lots of refreshing. A single move should be fine if the display is cleared.

@MrD-RC
Copy link
Member

MrD-RC commented Nov 2, 2025

Instead of having explicit functions to erase and draw elements. Why not just update the position and allow the next OSD redraw to handle the actual elements?

@Ahmed-hadzi
Copy link
Author

Thanks for the constructive feedback, but the use case for this would exactly be to constantly move the element around the screen. I was working on an AI tracking programme, and I needed a way for the companion computer to tell me what is it tracking at any given moment. Since I need to have my goggles on, I couldn't look at the companion telemetry or anything like that, so I needed to display it directly on the OSD. The way I intend to use it is that the AI tracking will run a few frames per second, and move custom element 1 to mark the object it's tracking in real time.

If I wanted to simply update a position of some object and then leave it there for a few minutes, I would use the already existing MSP command MSP_SET_OSD_CONFIG, which refreshes the whole OSD after updating position info, but also causes entire OSD to annoyingly flicker (flickering intensity depends on the goggles, the board etc.). My approach draws only moved object on its new position without any flickering or interference with other OSD objects, which can continue to do their job.

So, I can not refresh the entire OSD, as that would defeat the whole purpose of this, and I can't let OSD redraw handle it as it is too slow for this use case (it needs to be semi-realtime, maybe within half a second). For moving the standard elements, that would be too hard to implement because there are elements with variable lengths, and their lengths are not as accessible as lengths of custom elements. In order to accurately remove an OSD element, its characters are replaced by SYM_BLANK, but if the length of the element is not accessible there is no way to know which space to fill with blank characters.

@Ahmed-hadzi
Copy link
Author

The intended use case for this is dynamically updating OSD elements position while in-flight, without affecting pilot's ability to control the drone or view other critical OSD info, which would be made much more difficult if there was constant OSD flickering for full refresh every half a second. Also, I used Custom Elements because it's the easiest way to access and display custom icons on the OSD, for example icon 21 (up arrow) which can be used to mark the tracked object.

@MrD-RC
Copy link
Member

MrD-RC commented Nov 2, 2025

My issue is that this is for a very specific use case. For something that is in the release I feel like is should be less specific; like being able to update the position of any OSD element. A solution for this would that when the OSD element is drawn, there is an array of OSD elements length that is stored. Then a generic erase function could set those characters to SYM_BLANK, then change the position. The regular OSD draw would handle the rest. Maybe I'm overthinking it. But to me if you're adding something like this. Putting limits on it to a particular subset of the OSD seems like it can be avoided.

@Ahmed-hadzi
Copy link
Author

I agree that the use case is pretty specific, but I would also like to point out that AI object recognition in-flight is becoming more and more popular among developers, researchers and hobbyists.

I can't make this feature available for all OSD elements, as I do not have the necessary deep knowledge of how INAV or its OSD was intended to function, and it would take too much of my resources to do a deep research on the drawing mechanisms, specific element lengths etc. I feel like this feature only for Custom Elements is significant enough to be taken into consideration, since it is does not have any huge effects on INAV functionality while enabling this feature.

Also, drawing mechanism for every system is different (analog, DJI etc.), OSD elements' lengths are different, positioning is different and those are just too many unknowns for myself, as someone who is not that deeply familiar with INAV source code, to manage.

I proposed this only for Custom Elements because on them you can display basically anything, so they do not limit the user that much in terms of what can be moved, like it would be with some other elements. Most potential users, like me, would just use a Custom Element set to a single icon in order to highlight something on screen. With AI in drones becoming more and more popular, I believe this is a useful feature, which can also be updated and upgraded in the future if needed.

Anyways, if a user wants to move any OSD element in-flight not that often, and if they are ok with OSD refresh flicker, they can just use the already existing MSP command. My command is for dynamic in-flight frequent updates which are too frequent to deal with refresh flickering.

@sensei-hacker
Copy link
Member

Am I understanding correctly that it could be generic for different elements if only there was a function that gives the length of the element?

@Ahmed-hadzi
Copy link
Author

Yes. If there is a function which takes element ID as an argument, and returns the length of an element, I could make it work for any OSD element. Right now it works only with custom ones (they have the obvious variable prevLength), I couldn't find the equivalent for all other elements.

@error414
Copy link
Contributor

error414 commented Nov 3, 2025

How it looks like if moving custom OSD element overlaps other OSD element?

I would say that:

  1. fast moving custom OSD element could work as eraser, until other osd elements are redrawen
  2. if you hit bad time, custom osd element (for example arrow) can be overdrawn by OSD element what is originally on that position.

BTW: I would not allow to move other OSD elemets via MSP durring flight without whole screen refreshing, I would keep this functionality only for OSD custom elements

@Ahmed-hadzi Ahmed-hadzi requested a review from stronnag November 3, 2025 14:18
@Ahmed-hadzi
Copy link
Author

If OSD elements overlap, the field flashes between the 2 elements continuously at some interval, so both elements are pretty much visible. For example, if it's overlapping with battery voltage, the pilot should be able to see both the custom element and the voltage level. I don't think there will be problems with overdrawing, because my command is changing the item_pos value for a custom element, so that the OSD, when redrawing, thinks the element was supposed to be at that location in the first place, and will simply draw it again at the same coords.

I agree that allowing movement of all elements without refreshing the OSD may be too much, and I would also prefer to keep this feature for custom elements only, especially since there already is a MSP command that allows movement of any OSD element (if the pilot is ok with refresh-flicker). But, if there was a function I mentioned in the comment above, I could make it work for any element.

@sensei-hacker
Copy link
Member

sensei-hacker commented Nov 10, 2025

I was looking this over and I don't see a reasonable way to erase the old position of arbitrary elements. This is because you don't know the size of the element - not without significantly reworking (or duplicating) the drawing code.

The proposed MSP message does use the generic item id, so other elements could be implemented in the future. (Specifically single-line elements which use the draw string function would be straightforward).

@MrD-RC
Given that making it generic probably isn't a reasonable option, or would at least require significant changes to the current code, do you object to including this code? It would be nice if it was more generic, but I don't see that happening.

@MrD-RC
Copy link
Member

MrD-RC commented Nov 10, 2025

At the moment, even if it were just for custom elements. It could cause problems for other elements on the OSD, with erasing.

As it's for a very specific use case. I would be tempted to skip 9 so that it can be refined to cause less issues. Or because it's so specific, do we just let it in because no one is really using it?

Personally, I'm on the side of refinement. I think it could probably be handled better. But I need to check the code. And I really need to check the pan servos stuff as the next priority. As RC1 is getting close.

@Ahmed-hadzi
Copy link
Author

When I am testing it, it doesn't cause problems with other elements. Maybe it's different for other devices, but I am using MSP DisplayPort with DJI Goggles 2. When the moving element gets erased, the element it was overlapping doesn't get erased and stays on the screen.

I am open to refining the code if you find any issues, and possibly making it generic in the future.

@error414
Copy link
Contributor

could you share video? I'm curious how it works, and for what you use it

@sensei-hacker
Copy link
Member

As it's for a very specific use case. I would be tempted to skip 9 so that it can be refined to cause less issues. Or because it's so specific, do we just let it in because no one is really using it?

Yeah, I was thinking about this. I note it doesn't affect anything if you don't use it, so no harm. Ahmed has implemented the requested changes that can be implemented. I don't see any reasonable way to make it general. Perhaps as people see it and want to use it they'll find ways to improve it, based on their actual needs.

So I'm leaning toward go ahead and merge it, unless there's something that can be changed first and needs to be.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants