Driver for LX Navigation Eos57 #1284
Conversation
jirimaier
force-pushed
the
LX-Eos-Driver
branch
from
764370d
to
a604474
Compare
|
This PRs copies lots of existing code. That is not acceptable and will not get merged. |
MaxKellermann
added
the
needs-work
jirimaier
force-pushed
the
LX-Eos-Driver
branch
2 times, most recently
from
3dc8060
to
335c911
Compare
|
Thank you for the feedback. I modified the code to call the LXNAV driver's methods for parsing LXWP1 and LXWP3 sentences instead of copying the code (I think this was the main issue with copying existing code). I also modified the tests to use an LXWP1 sentence which is given as an example in the LX Navigation Communication Protocol documentation (instead of copying the one from the LXNAV driver test). I think that now there is no copied code. Some parts may be similar because the protocol is similar, but not exactly the same. I don't think that I could reuse other code from the LXNAV driver without making modifications to that driver. The Codacy Analysis shows issues regarding unused variables in src/Device/RecordedFlight.hpp, I believe these are false positives. These variables are used in DownloadFlight and GetFlightInfo methods of LXEosDevice. |
|
This ignores the vario's qnh setting. |
|
Also please test the driver post launch detection. I.e. with pressure on the tas. The vario behaves different post launch. |
The QNH of XCSoar is set based on the "alt_offset" from LXWP3 sentence. I am not sure what you mean by "varios autoqnh". When the vario is turned on, the user sets the current altitude (airport elevation), and the QNH of the vario is set accordingly. The altitude offset in LXWP3 can then be converted to the QNH setting so that the QNH in XCSoar gets set to the same value as on the vario. |
The alt_offset will remain the same value, after launch detection on the nmea side, no matter what you set the qnh on the varios ui to. |
|
I will test the in-flight behavior soon (probably tomorrow). |
|
If I understand the behavior of the vario correctly:
And you say that the alt_offset doesn't change when the QNH is changed in flight (This is a bug in the vario, right?). So this means that if the pilot changes QNH on the vario during flight, they also have to change QNH in the XCSoar. This is not so big problem I guess. I just need to make sure that the new (manually set) QNH in XCSoar doesn't get overwritten to the old value by the next LXWP3 sentence (that has outdated alt_offset). |
|
That summarizes it pretty well. |
|
I tested it in flight today. When changing QNH on the vario, the QNH in XCSoar also got changed. So it seems that the bug is not present in Eos. The baro altitude and FL shown by XCSoar always matched the values shown by vario, so the testing is successful. To make it compatible with the bug in Era, I can modify the LXWP3 parsing so that the QNH will be set only if the alt_offset is different from the one received in previous LXWP3 (or if it is the first sentence since connecting). Anyway, I noticed that if I set the QNH in XCSoar manually, all changes of QNH in the vario are ignored. |
|
It is a bit strange that in the initial setup of the vario, the elevation and QNH can be set independently. In XCSoar, the QNH and the difference between true and standard altitude are totaly dependent on each other. The alt_offset in LXWP3 only depends on the initial elevation and on in-flight changes of QNH, it doesn't depend on the initial QNH. If the initial QNH on vario is changed to different value than the one calculated from elevation (for whatever reason), the QNH in XCSoar (based on alt_offset) will be different from the QNH in vario. But this is fine, because the QNH in XCSoar will lead to correct conversion of true altitude to standard altitude,as it is based on the alt_offset. |
|
I noticed one more issue: If the Eos is connected together with another device (Flarm, for example), the altitude provided by the other dece is prioritized regardless of the order in the device list. In other words: the info.ProvideBaroAltitudeTrue(altitude_true) is "weaker" than info.ProvidePressureAltitude(value). Can this be considered a bug in XCSoar, or is it a feature? |
jirimaier
force-pushed
the
LX-Eos-Driver
branch
2 times, most recently
from
509b4d7
to
09671ff
Compare
| uint8_t prg[12]; | ||
|
|
||
| // TP Longitude in degrees multiplied by 60000.0f | ||
| int32_t lon[12]; |
There was a problem hiding this comment.
What's the byte order?
There was a problem hiding this comment.
big-endian, I added that to the comment
There was a problem hiding this comment.
Then why not use a safe class like PackedBE32 here? That way, it's impossible to misuse it, and you don't even need a comment.
|
|
||
| uint8_t bAutoNext = 1; // Is this auto next TP or AAT TP | ||
| uint8_t bIsLine = 0; // Is this line flag | ||
| float fA1; // Angle A1 in radians |
There was a problem hiding this comment.
Byte order? What bit format is being used? Is this bit format guaranteed on all CPU architectures?
| LXEosDevice::CalculateCRC(std::byte* msg, const int len, const uint8_t initial) | ||
| { | ||
| uint8_t result{ initial }; | ||
|
|
||
| for (int byte = 0; byte < len; byte++) { | ||
| uint8_t d = static_cast<uint8_t>(msg[byte]); | ||
| for (int count = 8; --count >= 0; d <<= 1) { | ||
| uint8_t tmp = result ^ d; | ||
| result <<= 1; | ||
| if ((tmp & 0x80) != 0) | ||
| result ^= 0x69; | ||
| } | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
Duplicate code, already exists in the LXNAV driver
| const uint8_t cmd = 0xF1; | ||
|
|
||
| // Flight ID is the index (1 = newest), not the ID from FlightInfo | ||
| uint16_t flight_id; |
| } | ||
|
|
||
| bool | ||
| LXEosDevice::GetFlightLogBlock(AllocatedArray<std::byte>& block, |
There was a problem hiding this comment.
This function should return the AllocatedArray instead of passing it by reference; throw an exception on error.
| std::memcpy(&julianDate, &response[13], 4); | ||
| std::memcpy(&time_takeoff, &response[17], 4); | ||
| std::memcpy(&time_landing, &response[21], 4); | ||
| std::memcpy(&file_size, &response[89], 4); |
There was a problem hiding this comment.
Byte order? Why are you using a byte array with hard-coded offsets here, instead of declaring a (documented) struct, as you did in other parts of this PR?
| env, | ||
| std::chrono::milliseconds(5000)); | ||
| } catch (...) { | ||
| return false; |
There was a problem hiding this comment.
What's the point of catching this exception? You discard all information about the failure by converting it tofalse here.
| uint8_t crc = CalculateCRC(responseHeader, sizeof(responseHeader), 0xFF); | ||
| crc = CalculateCRC(block.data(), block.size(), crc); | ||
| if (CalculateCRC(crc_byte, 1, crc) != 0) | ||
| return false; |
There was a problem hiding this comment.
Throw exception instead of returning false
| } | ||
|
|
||
| BrokenDate | ||
| LXEosDevice::julianToDate(uint32_t julian_date) |
There was a problem hiding this comment.
I think it could be useful to move this to class BrokenDate, e.g. BrokenDate::FromJulianDay(unsigned).
jirimaier
force-pushed
the
LX-Eos-Driver
branch
4 times, most recently
from
2dda702
to
910e893
Compare
jirimaier
force-pushed
the
LX-Eos-Driver
branch
9 times, most recently
from
b361cf7
to
6172135
Compare
jirimaier
force-pushed
the
LX-Eos-Driver
branch
6 times, most recently
from
78a732e
to
80e5cd8
Compare
Brief summary of the changes
Driver for LX Navigation Eos57 variometer.
Features
Compatibility
Known issues
Related issues and discussions
New generation LX Navigation Device #389
Wrong altitude from Lx Eos variometer #969
ERA80-Xcsoar #415