Skip to content
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

Add JSON (partial) #233

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Add JSON (partial) #233

wants to merge 12 commits into from

Conversation

lingfish
Copy link

@lingfish lingfish commented Oct 7, 2024

As per my comments in #190 this is a minimally-working attempt.

Again, I'm not a C programmer these days, and it might be rough. Hoping the devs or others can chip in and help out.

@EliasOenal
Copy link
Owner

Thanks for your PR. JSON output will be a great feature for multimon-ng, as many are building applications on top of it. But I am a bit unsure what the best pathway for an incomplete implementation is, that may need a bit more time to mature. Maybe we can keep it in its own branch and see if others want to polish it further. Do you have plans to keep working on it?

@lingfish
Copy link
Author

lingfish commented Nov 3, 2024

Hi sorry for the delays from me! Yeah, this feature I think is sorely needed -- JSON, or perhaps even a C API (meaning, being able to call C/multimon from say, Python). I'm using my branch at the moment in my mmng-ui project

Agreed re in it's own branch; my current contribution is tiny and far from complete. Should we perhaps move my branch to your repo, allowing others to contribute? Or keep it where it is?

I plan on continuing to work on it, but as stated, I haven't been a C/C++ programmer for 20+ years, so I could be wasting my and others' time writing terrible code.

@lingfish
Copy link
Author

lingfish commented Nov 9, 2024

image

gulp

@lingfish
Copy link
Author

lingfish commented Nov 13, 2024

My rough TODO list, based on uses of verbprintf(0, ...) calls:

  • clip.c
  • demod_dtmf.c (2 usages found)
  • demod_eas.c (13 usages found)
  • demod_flex.c (46 usages found)
  • demod_flex_next.c (52 usages found)
  • demod_morse.c (13 usages found)
  • fms.c (95 usages found)
  • hdlc.c (45 usages found)
  • pocsag.c (39 usages found)
  • selcall.c (5 usages found)
  • uart.c (7 usages found)

@lingfish
Copy link
Author

(BTW, I'm committing these individually so that if any comments are needed, they are easily put in context with whatever change I've done.)

@lingfish
Copy link
Author

Question time @EliasOenal -- of the remainder here, is a JSON mode even logical?

  • clip.c
  • demod_morse.c
  • fms.c
  • hdlc.c
  • selcall.c
  • uart.c

It kinda seems to me no? Except maybe morse.

@EliasOenal
Copy link
Owner

EliasOenal commented Nov 25, 2024

Thanks a lot for the hard work. Since we don't have better abstractions in place for the output generation, the resulting changes are a very verbose with the if-checks and cJSON calls. But that's surely not on you, we shouldn't have had that many prints to begin with. I'll perform some testing, but afterwards I'll be happy to merge it.

@lingfish if you don't mind, may we add your name? We try to attribute authorship properly to make it easier for the various package maintainers (Debian, Ubuntu, etc.) to track. Since the feature gets enabled in unixinput.c, could you put your name there below line 9? The distributions use automated tools to extract the names from all source files to list within the packages. But you may add it to the GPL headers of the other source files as well, if you want.

/*
 *      unixinput.c -- input sound samples
 *
 *      Copyright (C) 1996
 *          Thomas Sailer ([email protected], [email protected])
 *
 *      Copyright (C) 2012-2024
 *          Elias Oenal    ([email protected])
 *
 *      Copyright (C) 2024
 *          Your Name Here

Edit:
And regarding your question: yes, I think it would be useful for the other decoders as well. But with what's already done I think we can also merge this first and do the rest incrementally.

@lingfish
Copy link
Author

Thanks a lot for the hard work.

No problem.

Since we don't have better abstractions in place for the output generation, the resulting changes are a very verbose with the if-checks and cJSON calls. But that's surely not on you, we shouldn't have had that many prints to begin with.

Yeah I think perhaps a combo of feature-creep, and the different nature of incrementally printing to console, versus JSON output that needs to be batched and sent all at once. I did want to just hook into your main print function, but that would have been hard/need to buffer etc. A more major overhaul of it would be beyond my ancient C skills.

Since the feature gets enabled in unixinput.c, could you put your name there below line 9?

Will do.

Do you want me to squash a lot of commits here as well, or no?

And regarding your question: yes, I think it would be useful for the other decoders as well. But with what's already done I think we can also merge this first and do the rest incrementally.

Agreed.

@EliasOenal
Copy link
Owner

No need to squash commits unless you prefer so, I don't mind them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants