[BUG] Binary log msgpack uses non-string map keys

[seriyps]

Describe the bug
I'm not sure if WFB-ng's binary log in msgpack format is considered as part of public interfaces, but I'm working on using it for customized OSD OpenIPC/PixelPilot_rk#18
but got stuck on a fact that some of msgpack parsers can not parse msgpack with non-string map keys.
In Python it also requires strict_map_key=False.

'rx_ant_stats': {
  ((5805, 1, 20), 1): (661, -38, -37, -37, 8, 31, 37),
  ((5805, 1, 20), 0): (661, -42, -40, -40, 5, 30, 35)
}

To Reproduce

#include <nlohmann/json.hpp>

using json = nlohmann::json;
...
json::from_msgpack(buf, buf + size, false, true);

When buf contains the wfb-ng's binary log, this code will crash because library does not support non-string map keys.

Expected behavior

I think in order to make the binary log more compatible with various msgpack parsers, the format should either use maps with string keys or use array, eg

"rx_ant_stats": [
 {"ant": (5805, 1, 20), 1),
  "stats": (661, -38, -37, -37, 8, 31, 37)},
{"ant": (5805, 1, 20), 0),
 "stats": (661, -42, -40, -40, 5, 30, 35)}
]

Your setup (please complete the following information):

• OS Debian
• Hardware Radxa Zero 3W
• WiFi cards rtl8812eu
• WiFi drivers from FPV ground station for Radxa 3

Additional context
See OpenIPC/PixelPilot_rk#18

Confirm you read

• https://github.com/svpcom/wfb-ng/blob/master/README.md
• https://github.com/svpcom/wfb-ng/wiki/Setup-HOWTO

[svpcom]

Format with ((5805, 1, 20), 1) keys was selected due to possibility to have multiple statistic entries during TX frequency change. Conversion to json will not work, because json doesn't support non-string map keys. Have you tried native msgpack parsers like https://github.com/msgpack/msgpack-c ?

[seriyps]

Conversion to json will not work, because json doesn't support non-string map keys

@svpcom yes, because of that I proposed to replace the key-value dict with just a list:

"rx_ant_stats": [
 {"ant": (5805, 1, 20), 1),
  "stats": (661, -38, -37, -37, 8, 31, 37)},
{"ant": (5805, 1, 20), 0),
 "stats": (661, -42, -40, -40, 5, 30, 35)}
]

or maybe this to save space

"rx_ant_stats": [
 (
   (5805, 1, 20), 1),
   (661, -38, -37, -37, 8, 31, 37)
),
(
  (5805, 1, 20), 0),
  (661, -42, -40, -40, 5, 30, 35)
)
]

I looked at official parser yes, but the C API is way too lowlevel and C++ api depends on boost which is quite a serious dependency for just msgpack parser. Can end up using them though. I haven't checked if they work though. At least C one should be able to handle because it is low level.

[svpcom]

@seriyps ok. I can add additional stable JSON API for external programs. Let's discuss it structure

[seriyps]

Well, the data that we have in the binary log should be good enough. Maybe rather use named fields instead of positional like here

(661, -38, -37, -37, 8, 31, 37)

to be

{
count_all: 661,
rssi_avg: -38,
rssi_min: -37,
rssi_max: -37,
snr_avg: 8,
snr_min: 31,
snr_max: 37
}

Are you thinking about HTTP request API or some kind of event-sourcing?

[svpcom]

I'll prefer to have event-sourcing because HTTP is async and not very suitable for realtime streaming

[svpcom]

But on the python side I can generate any data format (except protobuf or other which requires a lot of external libs). The question what is easy to parse on the c++ client side

[seriyps]

Well, for my goals (to use detailed per-antenna radio stats for OSD) both size-prefixed JSON or msgpack would work just fine, the only issue was that current format uses non-string dict keys.
But if we want to make it more friendly for developers, I think streaming JSON objects separated by a newline would be slightly easier, because then it can be used een from bash (with jq).

One more thing that I believe might be nice to have in the initial (the one that is now type: cli_title) message - the command line/config options (like, channel/frequency range, FEC parameters etc) so one don't have to parse WFBs config to be able to show some of those params in OSD

[svpcom]

ok

[svpcom]

@seriyps Try json-api branch

[seriyps]

@svpcom thanks! Looks good at a first glance, I'll try to implement the OSD elements based on it and will let you know how it goes.

[seriyps]

Preliminary feedback:

generated by this code:

            data['rx_ant_stats'] = list((dict(zip(ka, (ant_id,) + k)), dict(zip(va, v)))
                                        for (k, ant_id), v in data.pop('rx_ant_stats').items())

data:

  "rx_ant_stats": [
    [
      {
        "ant": 1,
        "freq": 5805,
        "mcs": 1,
        "bw": 20
      },
      {
        "pkt_recv": 705,
        "rssi_min": -32,
        "rssi_avg": -29,
        "rssi_max": -28,
        "snr_min": 22,
        "snr_avg": 29,
        "snr_max": 36
      }
    ],
    [
      {
        "ant": 0,
        "freq": 5805,
        "mcs": 1,
        "bw": 20
      },
      {
        "pkt_recv": 705,
        "rssi_min": -12,
        "rssi_avg": -10,
        "rssi_max": -8,
        "snr_min": 22,
        "snr_avg": 30,
        "snr_max": 38
      }
    ]
  ]

it doesn't really make sense to have separate "key dict" and "val dict". I think they can be merged together:

  "rx_ant_stats": [
      {
        "ant": 1,
        "freq": 5805,
        "mcs": 1,
        "bw": 20
        "pkt_recv": 705,
        "rssi_min": -32,
        "rssi_avg": -29,
        "rssi_max": -28,
        "snr_min": 22,
        "snr_avg": 29,
        "snr_max": 36
      },
      {
        "ant": 0,
        "freq": 5805,
        "mcs": 1,
        "bw": 20
        "pkt_recv": 705,
        "rssi_min": -12,
        "rssi_avg": -10,
        "rssi_max": -8,
        "snr_min": 22,
        "snr_avg": 30,
        "snr_max": 38
      }
  ]

[seriyps]

@svpcom yep, seems to work quite well. I'm not using all the values yet, but quite many OpenIPC/PixelPilot_rk#41

I think the way it is now in your branch is good enough. Thanks!

[svpcom]

If no other values are needed then I can merge this branch

[seriyps]

It's good enough, thank you. One minor improvement might be to add a way to map antenna to network interface name, but it is minor.

[svpcom]

@seriyps added

[svpcom]

For clustered mode antenna mapping is in settings