g4klx / MMDVMHost

The host program for the MMDVM
GNU General Public License v2.0
370 stars 269 forks source link

call information in end of voice log line #519

Closed ZeroChaos- closed 4 years ago

ZeroChaos- commented 4 years ago

I was playing with parsing the MMDVMHost log and pulling radio stats out of the air while at a conference, and I found it extremely helpful to have the call information in the end of voice log line. I did it like this, but I'm assuming there is a better way to do it:

diff --git a/DMRSlot.cpp b/DMRSlot.cpp
index 9a6d7ec..7e0d4af 100644
--- a/DMRSlot.cpp
+++ b/DMRSlot.cpp
@@ -316,6 +316,10 @@ bool CDMRSlot::writeModem(unsigned char *data, unsigned int len)
                        // Regenerate the LC data
                        CDMRFullLC fullLC;
                        fullLC.encode(*m_rfLC, data + 2U, DT_TERMINATOR_WITH_LC);
+                       CDMRLC* lc = fullLC.decode(data + 2U, DT_TERMINATOR_WITH_LC);
+
+                       unsigned int srcId = lc->getSrcId();
+                       unsigned int dstId = lc->getDstId();

                        // Regenerate the Slot Type
                        slotType.getData(data + 2U);
@@ -339,7 +343,7 @@ bool CDMRSlot::writeModem(unsigned char *data, unsigned int len)
                        if (m_rssi != 0U)
                                LogMessage("DMR Slot %u, received RF end of voice transmission, %.1f seconds, BER: %.1f%%, RSSI: -%u/-%u/-%u dBm", m_slotNo, float(m_rfFrames) / 16.667F, float(m_rfErrs * 100U) / float(m_rfBits), m_minRSSI, m_maxRSSI, m_aveRSSI / m_rssiCount);
                        else
-                               LogMessage("DMR Slot %u, received RF end of voice transmission, %.1f seconds, BER: %.1f%%", m_slotNo, float(m_rfFrames) / 16.667F, float(m_rfErrs * 100U) / float(m_rfBits));
+                               LogMessage("DMR Slot %u, received RF end of voice transmission from %u to %u,  %.1f seconds, BER: %.1f%%", m_slotNo, srcId, dstId, float(m_rfFrames) / 16.667F, float(m_rfErrs * 100U) / float(m_rfBits));

Please consider making this change.

ZeroChaos- commented 4 years ago

I can make this a PR if desired, or you can directly take my change from here, or do it differently.

g4klx commented 4 years ago

That looks good. I am concerned that there may be a memory leak with the lc pointer, does it need to be deleted? I can't remember.

Secondly, this needs to be added to all modes (not just DMR), all types (voice/data) and also from RF and network. I like the idea a lot, but it needs to be system wide. A pull request would be the best way.

shawnchain commented 4 years ago

I would prefer to introduce a separate class like QSOLogger and spit all the conversation info into a tty file or unix socket for IPC, instead of a bunch of patches. Also, changing the log output will break the pi-star or mmdvm dashboard.

ZeroChaos- commented 4 years ago

I have the programming skill to PR this patch. This patch, which I wrote by brute force and I'm confident isn't "the right way" to do this. :-)

Basically all I want to do is parse out who is currently speaking, when they were done, and stats like BER and duration. If you want to introduce a different way to do this I'm likely to be perfectly happy with it.

g4klx commented 4 years ago

@shawnchain Remember that the code is cross platform and I will not include anything that isn't.

@ZeroChaos- Your code looks fine, it just needs a lot of duplication :-(

ZeroChaos- commented 4 years ago

@g4klx I literally made the code by copy pasta of things that looked similar to what I needed and testing it. I think this patch is like v23 of my tests. I don't code in C, and my style of copy pasta usually does horrible things like memory leaks or buffer overflows. Even if I make a PR, it won't be safe to merge without someone seriously looking at it. That said, I would be fine spending time prototyping some code and trying to match the output across the other modes if it helps you. You want to do it all, or review/fix my rough work?

g4klx commented 4 years ago

I'd be happy to look at any such code. I may try and do some work on it this weekend also, it depends on my time.

ZeroChaos- commented 4 years ago

After looking deeper into this, there are a lot more messages which could benefit from it. Additionally, it became obvious that I'm way out of my depth trying to do it. Some Example messages which could benefit from adding src and dst information if possible:

DMR Slot 2, audio sequence no. 0, errs: 0/141 (0.0%) DMR Slot 2, lost audio for 309ms filling in "DMR Slot %u, RF user has timed out", m_slotNo "DMR Slot %u, network user has timed out", m_slotNo

g4klx commented 4 years ago

I've added the extra data to all modes as well as for network also. This needs testing but should be about right.

ZeroChaos- commented 4 years ago

First of all, this is awesome, thanks for doing it.

While playing around I noticed two things: 1.) There are more messages which might benefit which I put in my previous comment. At least for dmr I believe every packet contains the from and to information, so adding it to the remaining messages should be possible. I know I want it, I might be the only one. :-) 2.) DMR Color code should also be on every packet, but I don't know how this is implemented in the code. At least for my dvmega, it seems to recieve all packets no matter what color code, and the configured color code is only actually used for transmission. But given color code is a configuration open do you even bother to decode it in the packet or it is just hardcoded to the user configured value? If it's decoded, I'd love it added to the messages, if it's hard coded, I don't need that.

Again, thanks for what you have already done, it's very helpful. In addition to having an led for transmit/receive (based on parsing the log file in realtime), I'd like to add a brightness fade based on loss percentage as well, but one of the things I'm detecting and logging is "interruptions" where one transmitter interrupts another, so having all the details in every message makes it way easier in such cases.

ZeroChaos- commented 4 years ago

The code is literal garbage, but if anyone actually cares about tracking stats this is what I'm doing right now: https://github.com/ZeroChaos-/dmrrr

I found this very useful while volunteering for a conference and running comms. To be super explicit, I'm not making money on any of this, nor do I have any intention of doing so, and I'm an extra class amateur radio operator.

g4klx commented 4 years ago

You make two points. Technically you still only need the QSO information at the start as you can then tie the following messages to the original by using the slot number. There will always be a QSO start message, however I have no problem with what has been changed. Any extra logging is strictly unnecessary.

The color code issue is only with the DVMega and you need to take that up with Guus. All MMDVM based systems are selective about the received color code so there is no need to log it. In fact (from what I remember), the color code is not decoded but simply matched with what I expect it to be based on the configuration.

ZeroChaos- commented 4 years ago

You make two points. Technically you still only need the QSO information at the start as you can then tie the following messages to the original by using the slot number. There will always be a QSO start message, however I have no problem with what has been changed. Any extra logging is strictly unnecessary.

I'm a little unclear on this so let me share my scenario with you and see if you think we need improvement or not. For example, I'm operating a repeater at a conference where multiple people may accidentally collide. In this case, user 1 may start transmitting and get cut in on by user 2 and maybe even user 3. As two or more users are transmitting at once, what would the log look like? Let's assume that the users are at approximately equal signal strength and we get the worst case of a perfect round robin of one packet out of three from each user. Basically we would be getting packets from all users "at once", and while I don't expect to perfectly piece all that back together, I would at minimum like to know it's happening and hopefully which users were involved in the collision. Is this something which can be derived from the current logs? Will I just get a bunch of starts and probably one end?

The color code issue is only with the DVMega and you need to take that up with Guus. All MMDVM based systems are selective about the received color code so there is no need to log it. In fact (from what I remember), the color code is not decoded but simply matched with what I expect it to be based on the configuration.

No problem, disregard the request to add color code, I agree it wouldn't be valuable

g4klx commented 4 years ago

On DMR radios are normally locked out if someone else is holding a slot. There may be cases where this happens but I think it'll be fairly rare. The MMDVM doesn't take kindly to someone else butting into a slot and would probably ignore them anyway. It may drop the other person out, and you'd get an end of transmission log message, then maybe a new message to say the new person had entered the system. I think your case is either unlikely or impossible.

ZeroChaos- commented 4 years ago

Oh my, I wish people configured their radios that way. In my experience they simply set admit criteria to "always". That said, the initial request has been fulfilled, and the additional discussion is academic at this point. I may open a new request after testing, but right now I'm simply going to call this solved.

Thanks for taking the time to explain how this works to me, it's been very helpful.