[groundcontrol]

Hi,

I just grabbed the source archive and tried to build, mostly went OK

make build_sdk_install  OK
make fw_resource   OK


until a way into make gcs

Code: [Select]

/~/librepilot/ground/gcs/src/libs/sdlgamepad/sdlgamepad.cpp:24:10: fatal error: SDL/SDL.h: No such file or directory
   24 | #include <SDL/SDL.h>   
Should I be able to at least build the current "next" at this time?

Thanks for any explanations/help.

(BTW do we really have to go through the pink pig routine every single time we post?  I thought that was just for signing up .)

[f5soh]

Hi,

You may need to install libsdl1.2-dev.

(BTW do we really have to go through the pink pig routine every single time we post?  I thought that was just for signing up .)

Yes, only requested at first post... too many spam.

[groundcontrol]

Hi, thanks for a speedy reply.

indeed, I was missing SDL headers,  it's SDL-devel on Fedora.   That seems to have got me flying ( no pun intended ).

That may need adding to the Fedora installation notes.

Also seems I need systemd-devel for libusb.h
The current pkg name for qtquick1 is : qt5-qtquick1-devel

[EDIT] Yes build completed fine. Thanks.

suggest adding systemd-devel  SDL-devel  and qt5-qtquick1-devel  to Fedora build notes.

[groundcontrol]

I don't see any kind of make install options in "make basics" and make package only seems to concern Gentoo and Ubuntu.

Is there any doc on what needs to go where once it's built?

Thx

[TheOtherCliff]

Developers often just leave the executables and libs where they are built and run it from there.

So, from command line inside the directory where you issue the make commands you just do something like:
./build/librepilot-gcs_release/bin/librepilot-gcs
(that is what it looks like in Linux anyway)

To run it from some other place, you should take the whole build directory and move / copy (and rename) it to some other location.  That will get GCS with all libraries, and also whatever firmware you have built.  At times I have done this to keep several old renamed build directories in case I needed them.

[groundcontrol]

To run it from some other place, you should take the whole build directory and move / copy (and rename) it to some other location.  That will get GCS with all libraries, and also whatever firmware you have built.  At times I have done this to keep several old renamed build directories in case I needed them.

Right, handy trick.

So, from command line inside the directory where you issue the make commands you just do something like:
./build/librepilot-gcs_release/bin/librepilot-gcs

Ah, that's what it meant.  Thanks.

Code: [Select]

make fw_resource
make gcs
build/librepilot-gcs_release/bin/librepilot-gcs
I could not work out what that last line was supposed to mean. I thought it was still part of build instructions.

Actually, I just checked back and I'm still getting a build error. Must be getting close now. 

Code: [Select]

/~/librepilot/ground/gcs/src/plugins/flightlog/flightlogmanager.cpp: In member function ‘void FlightLogManager::retrieveLogs(int)’:
/~/librepilot/ground/gcs/src/plugins/flightlog/flightlogmanager.cpp:232:76: error: taking address of rvalue [-fpermissive]
  232 |                             memcpy(&fields, &logEntry->getData().Data[start], header_len);
It points to the closing bracket of Data[start]

Is that a compiler version problem , gcc gets ever more rigorous about what loose programming it will accept. Here I'm running
gcc version 9.2.1 20190827 (Red Hat 9.2.1-1) (GCC)

Do you have a 'recommended' compiler version to work with?

    §5.3.1 Unary operators, Section 3

    The result of the unary & operator is a pointer to its operand. The operand shall be an lvalue or a qualified-id.

gcc correctly rejects this, Clang it seems just emits a warning. Judging by all the ^M chars I see in source it seems you are primarily working on Windows, presumably compiling with Clang.

gcc has the permissive switch , but setting that would obviously turn off error detection which may matter elsewhere. Maybe an temporary variable needs to be added to work around this and make it compile on both platforms.

[groundcontrol]

The following gets past the compiler.  However, I'm totally new to this code, could you check it over and make sure it using the right variable size etc.

If it looks good you may wish to include it to restore cross-platform building.

Code: [Select]

                       // cycle until there is space for another object
                        while (start + header_len + 1 < data_len) {
                            memset(&fields, 0xFF, total_len);
                            quint32 data_start = logEntry->getData().Data[start];
                            memcpy(&fields, &data_start, header_len);
                            // check wether a packed object is found
                            // note that empty data blocks are set as 0xFF in flight side to minimize flash wearing
                            // thus as soon as this read outside of used area, the test will fail as lenght would be 0xFFFF
                            quint32 toread = header_len + fields.Size;
                            if (!(toread + start > data_len)) {
                                memcpy(&fields, &data_start, toread);
                                ExtendedDebugLogEntry *subEntry = new ExtendedDebugLogEntry();
                                subEntry->setData(fields, m_objectManager);
                                m_logEntries << subEntry;
                            }
                            start += toread;
                        }


[TheOtherCliff]

I haven't looked at the code, but that isn't right. 

Think of Data as an array, given an integer offset/index into the array, you are trying to get the address of Data[start].

Instead of getting the address into data_start, this code puts the value at that location into data_start.

I would try something like:
memcpy(&fields, (char *)((int)(&(logEntry->getData().Data[start]))), header_len);
which I have not even tried to compile...  I suspect it will still throw an error since we are still taking the address.

The initial code seems well defined, and that it should work, so there is something else going on, like const'ness or static_cast or some other CPP headache that doesn't happen in C.  The correct fix may include changing the declaration.  Making it clear to CPP.

[groundcontrol]

Thanks, I should have looked at that a bit closer.

It seems the problem is that getData is a property 'getter' method, it is a function return value (on the stack) which needs assigning to something: it's an rvalue as the compiler rightly complains.
What the code is doing works but fails strict compiler rules.

So it seems there are two choices: either chose to slacken compiler checks (-fpermissive) which is probably not desirable generally of rework the code.

I suspect it will still throw an error since we are still taking the address.

Indeed.

The following compiles but I don't have h/w to do proper testing.

Code: [Select]

                            DebugLogEntry::DataFields tmpdata = logEntry->getData();
                            quint8* pdata_start = &tmpdata.Data[start];
                            memcpy(&fields, pdata_start, header_len);


[TheOtherCliff]

Well that looks reasonable, but it implies the original code was probably less efficient than it should be in that it returned the data not say a reference to it.

... Yea, just looked at the header and DataFields is a struct, not just a reference or pointer.  Looks like your code is one quick, reasonable way of fixing it.  It seems to me that for someone who knows whether it is difficult or not (and thus how painful it is), the correct way would be to have getData() return a reference or pointer (assuming that is reasonable).

On an only mildly related note:  The more syntactical candy they add to CPP, the more special cases they create that you need to know and the harder CPP is to use safely.

[groundcontrol]

I'm glad my second effort was nearer the target

Since 'next' does not currently compile with gcc , maybe someone familiar with the code can look at this and integrate the fix or come up with a better one.  I certainly would not want to get into deeper changes without a much better knowledge of the code base. Do you have folks doing that or it is this all code that was inherited from openPilot?

On an only mildly related note:  The more syntactical candy they add to CPP, the more special cases they create that you need to know and the harder CPP is to use safely.

I don't think this is 'candy'.  C was designed to be highly efficient and thus assumes a high degree of competence in the programmer in omitting a lot of safety checks. This was done intentionally because it was designed for writing operating systems, computer language compilers and such performance critical tasks.  Since C became the defacto standard for a lot of userland GUI stuff a lot of far less able folks were using it to knock up human interface software where code speed was really no important. This lead to all the bugs crashes and security issues we are now plagued with.  Mostly it was an inappropriate language choice for many of these tasks.

Object oriented languages are not designed to be ultra efficient: just consider all the object code which gets created to write a value to a simple integer 'property'.  You call a setter method , which typically calls the getter method, compares the new value to the existing value and if it changes then conditionally actually makes the assignment to the protected integer variable, and then calls the 'changed' method.

ie a whole  page of assembly language instructions instead of two or three.

Stricter syntax rules, eliminating things like the improper use of the rvalue seen here, are designed to remove some of the common sources of bugs and exploits which have become a major problem. I think there is a good reason for this kind of change and while it may be a pain removing the occasional line of dubious code, I think it is a move in the right direction, not "candy".

[TheOtherCliff]

<soapbox> 
Early on I loved C++.  Who wouldn't?  A better C.  Now C++ is such a big language with stuff being added every few years that I doubt there is anyone who knows it completely and can answer all questions correctly from memory, and that includes Stroustrup and people on the committees.  Part of the reason they keep tweaking is because new features expose new holes.  Take move semantics which has caused rethinking of lvalue rvalue into more categories.

After a couple new versions things have changed enough that you need to spend a good bit of time making sure you know it well even if your current job isn't using the new features.  I hope that your group has a good set of rules to keep the new graduates from needlessly spraying your code base with every new feature.  Old code that was legal must be modified to work with the new compiler.  If you change jobs, you need to get up to speed with which of the now many versions of C++ they use and the particular C++ quirks they use or abuse.

C++ is really very efficient; I'm not complaining about that at all.
</soapbox>