Add zmq server and json support by bkerler · Pull Request #102 · nccgroup/Sniffle

bkerler · 2024-10-12T13:06:18Z

This adds a zmq server, baudrate config and json output support, enabling external decoders

Update

mdxs · 2024-10-17T10:30:33Z

I think there are some inconsistencies in this PR:

Changes to the README regarding the ZMQ server port --smqport and host/ip (?) --smqhost command line parameters are different from the zmqsetting handling in the main routines of some of the other changed files. It is also not
There is no documentation in the README regarding the proposed introduction of the --baudrate command line parameter; while it isn't fully clear to me what the default would be if not specified (probably the same as pre-PR, but not clear); nor does it explain what the benefits or choices would be to modify this setting/parameter.

Furthermore, the PR adds numpy and scipy to the requirements for no obvious or described reason; as far as I can tell these are never used in the rest of the PR.

Wouldn't it make sense to split this PR into separate PRs for the introduction of the --baudrate by itself and another PR for the ZeroMQ (ZMQ) server? Perhaps the JSON output format is a pre-requisite for the ZMQ server, or would it by itself be an interesting feature to have a PR for just JSON output?

Hope this helps to improve or assess the PR(s).

mdxs · 2024-10-17T10:31:27Z

python_cli/advertiser.py


    global hw
-    hw = SniffleHW(args.serport)
+    hw = SniffleHW(args.serport,baudrate=args.baudrate)


Missing a space after the comma, after: ...(args.serport,

mdxs · 2024-10-17T10:43:05Z

python_cli/scanner.py


    global hw
-    hw = make_sniffle_hw(args.serport)
+    hw = make_sniffle_hw(serport=args.serport,baudrate=args.baudrate)


Missing a space after the comma, after: ...(args.serport,

mdxs · 2024-10-17T10:49:05Z

Next to the above highlighted whitespace issues in the code, the changes proposed to packet_decoder.py seem to have several whitespace issues. Not sure if that is not the case in Sniffle in other places, but there seem to be a lot in this PR in that specific file.

bkerler · 2024-10-17T13:07:04Z

@mdxs

Furthermore, the PR adds numpy and scipy to the requirements for no obvious or described reason; as far as I can tell these are never used in the rest of the PR.

python_cli/sniffle/resampler.py. Both numpy and scipy were already required.

bkerler · 2024-10-17T13:09:46Z

@mdxs

There is no documentation in the README regarding the proposed introduction of the --baudrate command line parameter; while it isn't fully clear to me what the default would be if not specified (probably the same as pre-PR, but not clear); nor does it explain what the benefits or choices would be to modify this setting/parameter.

In fact, the README already states that the firmware differs in its baudrate depending which firmware was flashed:

To install Sniffle on a SONOFF CC2652P dongle (equipped with a CP2102 USB/UART bridge), you need to use a special firmware build that uses a 921600 baud rate (labelled 1M) instead of the default 2 megabit baud rate.

mdxs · 2024-10-17T13:51:49Z

README.md

  -o OUTPUT, --output OUTPUT
                        PCAP output file name
+  -z, --zmq             Enable ZMQ server
+  --zmqsetting          Set ZMQ server ip and port (default:127.0.0.1:4222)


Adding a space after default: should make this more readable (and as it would be rendered in fact?): ... and port (default: 127.0.0.1:4222)

Changed that accordingly.

mdxs · 2024-10-17T14:01:45Z

In fact, the README already states that the firmware differs in its baudrate depending which firmware was flashed:

As I'm not using/needing this, I might not have been aware of this and may never have needed it. I will leave it to you and others to decide if it is clear (from the other documentation) what value should be used by someone that does need it and in which situation(s). Could Sniffle figure it out from the device using some FW version details? Hmm, perhaps it cannot reliably if there is a mismatch from the start and the communication doesn't work. Oh well, outside my level of use/understanding.

mdxs · 2024-10-17T14:04:06Z

Next to the above highlighted whitespace issues in the code, the changes proposed to packet_decoder.py seem to have several whitespace issues. Not sure if that is not the case in Sniffle in other places, but there seem to be a lot in this PR in that specific file.

This appears to have been addressed as well, as I didn't spot whitespace issues in packet_decoder.py now.

mdxs · 2024-10-17T14:15:51Z

Note that I just looked at the PR and reported my observations, I didn't try to run it at all... so it is just an eyeball review.

I still wonder about the mixing of different topics into one PR, but will leave that to @sultanqasim to decide.

XenoKovah · 2024-10-17T20:38:32Z

python_cli/sniffle/sniffle_hw.py

        elif is_cp2102(serport):
-            baud = 921600
+            if baudrate is None:
+                baud = 921600


I think this could be removed as part of a reversion to 2Mbaud everywhere as requested in #101 , and then just letting people use the new baudrate option this introduces if they want 921600.

Without that, it isn't working with the current code, as other hw doesn't support 2Mbaud.

sultanqasim · 2024-10-25T02:42:31Z

Thanks for your contributions. Sorry for my delayed response, as I just had my first kid born and she has been keeping me busy :)

I'll probably split these changes into a few commits for the different things in here (baud rate selection, mark and flush timeout, the ZeroMQ server, and cosmetic changes). I may skip the cosmetic changes and implement the mark and flush timeout a bit differently.

sultanqasim · 2024-10-25T02:46:11Z

@mdxs

Furthermore, the PR adds numpy and scipy to the requirements for no obvious or described reason; as far as I can tell these are never used in the rest of the PR.

python_cli/sniffle/resampler.py. Both numpy and scipy were already required.

Numpy and Scipy are not listed as dependencies in the README because they are only used for the experimental SDR-based sniffing functionality that I haven't yet documented and is very much an incomplete work-in-progress. You don't need Numpy or Scipy for the TI CC13xx/CC26xx based sniffer.

sultanqasim · 2025-01-10T21:45:10Z

Rebased to avoid conflict with #106

Bjoern Kerler added 8 commits July 25, 2024 20:28

Add OpenDroneID

03b3716

Merge branch 'OpenDroneID' into master

19d70ba

Merge pull request #3 from bkerler/master

506b337

Update

Fix baudrate

e1728d5

Move opendroneid part to separate repo, enable zmq listener

9047507

Update

97f7d0f

Update

e657d70

Merge branch 'nccgroup-master2' into OpenDroneID

253fa7a

mdxs reviewed Oct 17, 2024

View reviewed changes

bkerler force-pushed the zmq branch from 795a70c to d481099 Compare October 17, 2024 13:23

mdxs reviewed Oct 17, 2024

View reviewed changes

mdxs mentioned this pull request Oct 17, 2024

Default to 2Mbaud for Sonoffs? #101

Closed

XenoKovah reviewed Oct 17, 2024

View reviewed changes

bkerler force-pushed the zmq branch 2 times, most recently from 65098d8 to 5aa3972 Compare October 20, 2024 16:22

sultanqasim force-pushed the zmq branch from 5aa3972 to 192ac49 Compare January 10, 2025 21:44

Add zmq server and json support

1b16c3f

sultanqasim force-pushed the zmq branch from 192ac49 to 1b16c3f Compare January 12, 2025 16:19

Update

dae3940

Bjoern Kerler and others added 5 commits August 18, 2025 16:03

Merge branch 'nccgroup-master' into zmq

cb961fe

Merge branch 'OpenDroneID' into zmq

c727afb

Merge branch 'nccgroup:master' into zmq

affef63

Fix indention

06e9da4

Update version, add bluetooth_receiver python script, change permissions

957603d

Conversation

bkerler commented Oct 12, 2024

Uh oh!

mdxs commented Oct 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdxs commented Oct 17, 2024

Uh oh!

bkerler commented Oct 17, 2024

Uh oh!

bkerler commented Oct 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdxs commented Oct 17, 2024

Uh oh!

mdxs commented Oct 17, 2024

Uh oh!

mdxs commented Oct 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sultanqasim commented Oct 25, 2024

Uh oh!

sultanqasim commented Oct 25, 2024

Uh oh!

sultanqasim commented Jan 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants