Openvario / variod

Daemon for autonomous e-vario
4 stars 8 forks source link

make communication protocol complient with XCSoar and fix some bugs #4

Closed bomilkar closed 4 years ago

bomilkar commented 4 years ago
  1. introduced support for POV,C,RPO,... and POV,C,IPO,.... consitent with documentation

  2. leave POV,C,POL,... as undocumented feature; should be eliminated lateron

  3. prefer Real Polar information (POV,C,RPO,...) over Ideal Polar with later calculation of real polar. The overload information from XCSoar is not correct if refernce_mass != dry_mass . Therefor the information is not enough to calculate real polar from ideal polar. And XCSoar does it all for as and provides it ready to use in POV,C,RPO,...

  4. parse_NMEA_command() in variod.c porcessed the incomming sentences in reverse order if several sentences came with one recv().

bomilkar commented 4 years ago

Please note that variod needs some meaningful NMEA data from sensor. It is currently not OK to run sensord without sensors. It may (or will) misbehave on the socket. If you don't have a working sensord (because you are testing on a platform which doesn't have any sensors, you should run a sensord-mock-up instead of sensord. The source for that (very simple) tool is attached. TODO: fix sensord such that it doesn't misbehave if there is no proper set of sensors available.

bomilkar commented 4 years ago

Here is the source for this sensord dummy. sensord_mock.c.txt

On Debian 10: "gcc -o sensord_mock sensord_mock.c" For the current build environment on Cubie2 use the compile script. compile_sensord_mock.txt

kedder commented 4 years ago

Ugh, reformatting it all made it worse, please don't do it. Just use tabs for indentation, like it was originally.

bomilkar commented 4 years ago

reformatting it all made it worse

I kept the format in variod.c as much as it was possible. However, in nmea_parser.c there are too many scattered changes to bring it back the previous indentation style. It just gets worse in diff.

bomilkar commented 4 years ago

This is it at least from my perspective. A number of checks have been added to nmea_parser.c to make sure the checksum is correct and the sentence has enough values. This reduces the probability that corrupted/incomplete sentences can affect the system's parameter (Polar, MC, ...) I can't see the issue described in here https://github.com/Openvario/meta-openvario/issues/57 anymore. Part of variod.log :

Get Polar IPO: 0.002376, -0.103800, 1.800000
Get Polar RPO: 0.002515, -0.115333, 2.098999
Get Ballast Value: 1.000000
Get Ballast Value: 1.013000
Get Ballast Value: 1.026000
Get Ballast Value: 1.039000
Get Ballast Value: 1.053000
Get Ballast Value: 1.066000
Get Ballast Value: 1.079000
Get Polar RPO: 0.002422, -0.115333, 2.180281
Get McCready Value: 1.000000
Get McCready Value: 1.500000
Volume up
Volume down
Toggle Mute
Volume up
Volume down
Toggle Mute
Toggle Mute
Toggle Mute
Toggle Mute
Volume down
Volume up
bomilkar commented 4 years ago

Just to be precise:

  1. rename mv NMEAchecksum.c nmea_checksum.c and mv NMEAchecksum.h nmea_checksum.h
  2. for vi I use ":set noexpandtab" and ":set tabstop=8" and ": set autoindent"on 4 files nmea_checksum.c, nmea_checksum.h, nmea_parser.c and variod.c
  3. change function and variable names in nmea_checksum.c, and nmea_checksum.h.
  4. leave the rest untouched

@kedder please confirm. Is this what you want?

kedder commented 4 years ago

@bomilkar that sounds reasonable, except vim commands you menitioned will not change any existing indentation. You might want to look at :help retab.

bomilkar commented 4 years ago

I give up.

kedder commented 4 years ago

I give up.

Why?

bomilkar commented 4 years ago

Because it doesn't merge anymore.

kedder commented 4 years ago

You just have to resolve the conflict.

bomilkar commented 4 years ago

After the merge XCSoar doesn't connect when it starts. You have to go to Devices and hit "Reconnect".

bomilkar commented 4 years ago

I think I found the issue in the code. @iglesiasmarianod shouldn't the create_xcsoar_connection(); be inside the while(1) loop?

iglesiasmarianod commented 4 years ago

It is outside as well as inside. Inside works only if the send message fails. I've compiled test image after merging and can't reproduce your problem. I'm testing in the actual OV with sensors. Perhaps the difference is there? In my device it connects on startup and after quiting XCSoar reconnects on restart.

bomilkar commented 4 years ago

Think about it: you close xcsoar_sockat the bottom of the infinite loop. You should open xcsoar_sockat the top of this while(1) loop, not before it. Admitted: it will eventually open xcsoar_sockbut not unless send message fails. (And then you are closing a socket which has already been closed in this scenario.)

We can fix this issue if this PR https://github.com/Openvario/variod/pull/4 has been merged, if it ever merges.

kedder commented 4 years ago

You should open xcsoar_sock at the top of this while(1) loop, not before it.

I don't understand this part. Why would you establish xcsoar connection in a loop?

bomilkar commented 4 years ago

I was mistaken: I saw close(xcsoar_sock); inside the endless loop. In fact it is outside (unreachable). variod works as expected, at least on my debian 10 system. I'll test it on OV tomorrow. I'll double check if the Volume Up/Down/Mute works. Now it would be nice to get the counterpart merged: https://github.com/XCSoar/XCSoar/pull/378

bomilkar commented 4 years ago

I've tested a lot on the OV system using my sensord_mock to supply POV sentences. It all looks good. So I consider this done. I will delete the branch tomorrow unless someone objects.

iglesiasmarianod commented 4 years ago

I've updated and rebuild the full image to last merge. Everything works fine except mute/unmute in my device. Using OV5,7 image with actual sensors.

bomilkar commented 4 years ago

That is strange. Can you check in the log file? (/variod.log) Depending on which buttons I press, I see this:

Volume up
Volume down
Toggle Mute
Volume up
Volume down
Toggle Mute
Toggle Mute
Toggle Mute
Toggle Mute
Volume down
Volume up

And the audio output of the Cubie sounds exactly like this.

iglesiasmarianod commented 4 years ago

Yes, I can see toggle mute in the log but audio is still beeping. It was strange that the Switch Case parsed Volume Up and Volume Down the right way and Toggle mute the wrong way being in the same statement. I believe that If this problem were message based the malfunction would have been random. Some times volume up might fail, sometimes volume down might fail, etc, depending on where the message was mistranssmitted or misintrepreted. Being a sistematic error (with old parsing as well as with new parsing) the problem must lie elsewhere. That's why I asked if toggle_mute() function was doing its work. Just in case checked XCSoar audio and it is off.

bomilkar commented 4 years ago

The previous version wasn't able to parse the nmea sentences correctly, especially when recv() came back with more than one sentence. That lead to sentences being dropped or corrupted. Also nearly no checks were performed on sentences. It was easy to process incomplete sentences.

If you have seen the expected tokens in the log file, then the issue must me somewhere else. toggle_mute() is trivial. The work is being done inside pulseaudio, I beleive. So far I had no reason to look there. So I left the lid on.

iglesiasmarianod commented 4 years ago

Toggle_mute works the same way as volume_mute() and volume_unmute(). They change the variable "mute" from true to false. If mute and unmute work from variod (you call unmute on start, call mute on XCSoar close, and unmute again on XCSoar restart). Why toggle still doesn't work? Dont think the problem is in pulseaudio or the parsing. Looking quickly to Variod I saw the unmute function is called endlessly inside the while loop. Think the problem is there. It unmutes all the time regardless what the command says in over and over again. I'll change that and test as soon as I can.

bomilkar commented 4 years ago

It shouldn't get vario_unmute(); in line 287 unless the sockets break, which should not happen once it's running. It should not leave the while ((read_size = recv(.... ) starting in line 293. And if you'd get to line 287, you would see fprintf(fp_console, "Connection accepted\n"); in the log. I don't see that.

iglesiasmarianod commented 4 years ago

Please look at While(1).