-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
FlySky Ibus telemetry support #2415
Conversation
Enable with cli |
👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice code, I just commented few details.
Only problem I see is frame sync detection, it does work, but better solutions probably exist.
I did not check unittests closely, but they look fine
|
||
static uint8_t ibusReceiveBuffer[IBUS_RX_BUF_LEN] = { 0x0 }; | ||
|
||
static uint16_t calculateChecksum(uint8_t ibusPacket[static IBUS_CHECKSUM_SIZE], size_t packetLength) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static
does not make any sense here (I am surprised it's even syntactically correct)
And ibus packet length is not IBUS_CHECKSUM_SIZE
.
const uint8_t *ibusPacket
is normal way to pass array in C
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Static global variables are not visible outside of the C file they are defined in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, did not know this part of C:
http://stackoverflow.com/questions/3430315/what-is-the-purpose-of-static-keyword-in-array-parameter-of-function-like-char
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
..and my reply was not for the static inside the array index. I'm also sruprised it was there. Anyway it's gone now since I updated the code with the other review comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well ... minimum size check is nice feature, but it is not used in CF yet ...
return checksum; | ||
} | ||
|
||
static bool isChecksumOk(uint8_t ibusPacket[static IBUS_CHECKSUM_SIZE], size_t packetLength, uint16_t calcuatedChecksum) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider calculating checksum inside this function - this should check packet correctness, caller does not need to know details
} | ||
} | ||
|
||
static void sendIbusCommand(ibusAddress_t address) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is discover reply, not command
|
||
static void sendIbusCommand(ibusAddress_t address) { | ||
uint8_t sendBuffer[] = { 0x04, IBUS_COMMAND_DISCOVER_SENSOR | address, 0x00, 0x00 }; | ||
transmitIbusPacket(sendBuffer, sizeof sendBuffer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in CF, sizeof
is used with function syntax, not operator: sizeof(type)
IBUS_SENSOR_TYPE_EXTERNAL_VOLTAGE = 0x03 | ||
} ibusSensorType_e; | ||
|
||
static const uint8_t SENSOR_ADDRESS_TYPE_LOOKUP[] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CAPITALS are used (only) for constants.
case IBUS_SENSOR_TYPE_TEMPERATURE: | ||
{ | ||
#ifdef BARO | ||
float temperature = (baroTemperature + 50) / 100.f; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like integer rounding - but it will not work for floats (and will work incorrectly for negative values)
If you want to round the value, do so explicitly (roundf()
) after final scaling.
#else | ||
float temperature = telemTemperature1 / 10.f; | ||
#endif | ||
sendIbusMeasurement(address, (uint16_t) ((temperature + 40)*10)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Valu is converted to degrees C above and then back to decidegrees here. With double conversion avoided, all calculations can be integer based.
} | ||
|
||
if ((returnAddress >= ibusBaseAddress) && | ||
(ibusAddress_t)(returnAddress - ibusBaseAddress) < sizeof SENSOR_ADDRESS_TYPE_LOOKUP) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better use ARRAYLEN(SENSOR_ADDRESS_TYPE_LOOKUP)
, it will handle larger types corectly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you mean something like:
#define ARRAYLEN(array) (sizeof(array)/sizeof(array[0]))
Is that available in c? or defined somewhere in cleanflight?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uups, did a file search on the wrong repo, thanks
} | ||
|
||
static void pushOntoTail(uint8_t buffer[IBUS_MIN_LEN], size_t bufferLength, uint8_t value) { | ||
for (size_t i = 0; i < bufferLength - 1; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
memmove
should be faster
|
||
while (serialRxBytesWaiting(ibusSerialPort) > 0) { | ||
uint8_t c = serialRead(ibusSerialPort); | ||
pushOntoTail(ibusReceiveBuffer, IBUS_RX_BUF_LEN, c); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is quite fragile (it will be hard to support longer request packet) and is a bit suboptimal (each packet is tested four times / processed data are scanned again)
Interpacket gap delay be better (SBUS and others use that) ..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that its not perfect, but I'll leave it be for now.
The packets are maximum 4 bytes so the impact is not that big compared to the time I'm willing to put into this at the moment.
In the future it would be more interesting to see if ibus serial rx and telemetry could coexist on the same serial port with some added hardware (a resistor and diode at the least).
#ifdef BARO | ||
float temperature = (baroTemperature + 50) / 100.f; | ||
uint16_t temperature = (baroTemperature + 50) / 10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(baroTemperature + 5) / 10
or just baroTemperature / 10
And type is int16_t
(or even better int
, gcc will generate better code)
It was probably used to round correctly (2160 centidegrees to 22 degrees). But it will return invalid results for negative values (division rounds toward zero). 0.1C is much smaller that sensor accuracy, it's OK to ignore rounding ..
This code will round correctly, but is a bit of overkill:
temperature = baroTemperature / 5; // divide in two steps
temperature = (temperature + 1) >> 2; // left shift rounds toward negative infinity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic is just copied from the file: src/main/telemetry/frsky.c
hope that its ok now in the latest commit. I'd love if anyone could try it for real now after all changes.
Thanks for the review by the way!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Frsky code rounds the value, it works fine for non-negative temperatures. But added value has to be half the divisor.
baroTemperature / 10
should do in iBus case.
|
||
case IBUS_SENSOR_TYPE_TEMPERATURE: | ||
#ifdef BARO | ||
value = (baroTemperature + 50) / 10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
value = baroTemperature / 10;
With your version reading will be 0.5C too high
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mojocorp Any input on this? It's your implementation from the beginning? (according to a comment in the old pull request)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@unitware It's a copy/paste from the frsky telemetry. No idea where the +50 is coming from.
As far as I remember the git history doesn't help at all and there is no mention anywhere about the units (I assume degrees centigrades). The temperature should be sent as DECIGRADES to the ibus telemetry, that's why there is a division by 10.
I've fixed the gyro temperature reading for the naze32 (MPU6500) and my pull request was merged last month. Maybe using the gyro temp should be enough.
Anyway, the temperature coming from the gyro (MPU6500) is in degree centigrades. I have no idea for other gyro (no units anywhere).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, so now I am confused. I am removing the 0.5 degree offset because its probably a manual fix to deal with die temperature etc.
My best effort is now something like this.
case IBUS_SENSOR_TYPE_TEMPERATURE:
#ifdef BARO
value = baroTemperature / 10;
#else
value = telemTemperature1 * 10;
#endif
sendIbusMeasurement(address, value + IBUS_TEMPERATURE_OFFSET);
break;
Is it then correct to assume that:
- baroTemperature is in centidegrees / 10 -> decidegrees.
- telemTemperature1 is in degrees * 10 -> decidegrees.
@mojocorp you said that gyro temp is in centidegrees, did you mean baroTemperature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 0.5C offset is because original code did compute value in degrees. Integer division in C is rounding positive numbers down (190/100 == 1), with this offset resulting (positive) value is correct. Please read my earlier comments ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, thanks. I did not get that at that time (even if I read it) :)
Recommendation documentation updates: Supported RX device
|
Thanks, what about PPM support? How many channels? Skickat från min iPhone
|
@unitware already updated. |
@unitware Found BUG in FS-iA4B , Bind may be lost . (Not recommended) |
@cj1324 Did you run the saleae on that rx as well? Thinking is it a bug in the rx or in the our code? Does it work the same way as the others? |
@unitware Suggested: return FS-iA4B In addition: |
Ok, the new rx is without ibus telemetry it seems. Skickat från min iPhone
|
@hydra and other admins, Is there any chance of getting a merge before the conflicts start to build up? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this is ready to merge
\_______/ | ||
``` | ||
|
||
It should be possible to daisy chain multiple sensors with ibus. This is implemented but not tested because i don't have one of the sensors to test with. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FC must be last in the chain, it may be mentioned here
IBUS_SENSOR_TYPE_EXTERNAL_VOLTAGE = 0x03 | ||
} ibusSensorType_e; | ||
|
||
static const uint8_t sensorAddressTypeLookup[] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe comment that Address n
is for single sensor case (not chained / chained only with RX internal voltage)
Or change it to A+0 / A+1 / A+2 when placed at position A in chain
And IBUS_SENSOR_TYPE_INTERNAL_VOLTAGE is not sent by this sensor(FC), so it should not be here
@@ -69,6 +70,9 @@ void telemetryInit(void) | |||
initSmartPortTelemetry(); | |||
initLtmTelemetry(); | |||
initMAVLinkTelemetry(); | |||
#ifdef TELEMETRY_IBUS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indenting is inconsistent with rest of (or larger part of) code here - #directive
should be in leftmost column / use # directive
to nest #if
s etc ..
Conditional code is normally indented:
initMAVLinkTelemetry();
- #ifdef TELEMETRY_IBUS
Write Preview
Indenting is wrong here - #directive
should be in leftmost column / use # directive
to nest #if
s etc ..
Conditional code is normally indented:
initMAVLinkTelemetry();
#ifdef TELEMETRY_IBUS
initIbusTelemetry();
#endif
telemetryCheckState();
Ehm, I got my own logic analyzer today and hooked it up. It seems like I need to filter out the stuff that the FC is transmitting since it will echo back again. The packets are the same in some cases so after the first sensor discovery packet the bus will flood with them. Actually it is strange that it works as well as it does. I'll have a look at it tonight So do not merge just yet.. Update: I also did a rebase to the current master since there were conflicts. |
8ec99dc
to
34ca677
Compare
please rebase again i just pushed a few changes and merged some PR's. |
34ca677
to
dd01488
Compare
IMO
|
@ledvinap thanks for checking the code I thought of your suggested alternatives but went for the current one since it felt more safe for me in the respect that it does not affect the drivers - thus not destroying any other functionality. I'm not that familiar with the full codebase to do that safely and with confidence, the current changes could be made within the safety harness of the unit tests. If you want then please elaborate why you think the counter solution being fragile, it's always interesting to get feedback! If you or anyone else wants to have a go at doing it differently please do so and either create a pull request towards my ibus-telemetry branch or to cleanflight if this PR has been merged. I can do some testing on it if need be. A side note: If you do and start thinking of inter packet timeout as a synchronization then take into account that a possible improvement to this is to combine flysky serial RX with the telemetry on the same serial port, it looks like it could be done with some hardware adaption. That's probably another PR, but the gap in between packets will e much smaller, in the vicinity of 1ms. |
@unitware : Discarding all received chars just after |
@ledvinap : |
@unitware : Sorry, I overlooked the TX buffering problem ... |
You solution is probably only reasonable given current serial driver state ... |
a020587
to
00ef0d6
Compare
so, the lack of flight testing scares me! Has anyone actually flown this code? |
00ef0d6
to
def22c8
Compare
I will merge this code now, I want to get a new release candidate ready today for people to test this weekend. Please provide feedback on this PR once you've tested it and it was OK, if not OK please raise new issues and PR's as appropriate. |
|
I have a SP Racing F3 and a FS-i6 and FS-iA6b hardware. I'm using IBUS on UART3 (just RX pin is connected to IBUS Signal on the FS-iA6b). How can I test this? |
danarrib, you need to connect TX pin of UART to I-Bus sensor pin of FS-iA6B, like doc says https://github.com/cleanflight/cleanflight/blob/master/docs/Telemetry.md |
@rootik cool pictures, maybe do a PR with updated documentation? Thanks for testing, remember that soft serial is not guaranteed to work with 115200 baud rate (unless I'm mistaken) |
@unitware, I have not yet had the opportunity to test it in flight, but it definitely works on bench. |
By the way, all possible sensors types supported by FS-i6 TX are: Mot Tem ExtV Odo1 Odo2 Spe and reserved - IntV Tx.V Err 0x00 is for "IntV", internal voltage |
Ok @rootik . Thanks for the information. Can I enable "Telemetry" and "Serial RX" on same port in the ports configuration? Or it must be on separate ports? |
@danarrib, no, you can't. It must be separate ports. |
Ok, Thanks. I'll test it on the weekend. Can someone please provide a .hex file for SP Racing F3 Deluxe? |
Currently testing the ibus feature under naze32 (on softserial). I'm getting these sensor available and working:
I'm seeing that "Mot 4" sensor sometimes it disappear then it appear again. The others are stable. I got two question for you:
Thank you. inode |
@iNode Hi, and thanks for testing! I have not tested ibus telemetry on softserial myself. The baudrate is high and there is a requirement to respond within a certain short time (unclear how long time that is, but within the range of a ms I think). This might cause the disappearing sensor. To answer your questions:
Unless you are willing to do the coding yourself, then open an issue to get some more people on the idea and discuss possible solutions, and maybe someone who is willing to implement it. I'm sorry to say that I will not find the time to do so in the coming month at least. |
@unitware on your tests the receiver ask via the ibus only the IDs that he know or ask all the IDs from 0x00 to 0x7F? I'm asking you that to understand better the situation. I was thinking about create an addon to the received to manager the telemetry data not managed by the FS-I6 and show them on a little display. Known id: |
Test on FS-i6S and iA6B. test result: |
More tests executed, I hope tomorrow to execute a flying test. My testcase is configured with 2 softserial, the first connected to an ubox at 19200 baud, the second used for telemetry. Used CLI to configure the serial port setting (serial 31 1024 115200 57600 115200 115200). Here the results:
This so high CPU load can cause problem with on fly tests? |
.report_cell_voltage = false, | ||
); | ||
|
||
#define IBUS_TASK_PERIOD_US (500) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iNode
Regarding the cpu load, you can try to increase this value in steps until you see the telemetry stop working and then back off a bit until it is working again. Then see if you can get GPS online again.
@cj1324 I know this is a very old discussion, but... The "Sig.S" telemetry info that appears in your FS-i6S with a value of 10, what does it means? My FS-I6S has the same data, maybe some info about signal? |
@McGiverGim Should be the signal value, did not find the official description. |
@cj1324 Thanks! |
This is an enhanced version of @CraigJPerry pull request #1723 dealing with issue
#1125
My contributions:
I have tested the code in my quad but not flown much with it since I have barely set up my PID:s. I'd really like someone more experienced in flying to take a test run to see if stability or other things have been affected.
Credits to @CraigJPerry et.al.
Update:
@ledvinap has given lots of good review comments which are implemented except for the frame detection.
@cj1324 has done some bench testing with different receivers, added to documentation.