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.

Sign up for GitHub

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

Jump to bottom

Add zmq server and json support #102

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add zmq server and json support #102

wants to merge 1 commit into from

Conversation

bkerler
Copy link

@bkerler bkerler commented Oct 12, 2024

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

@mdxs
Copy link
Contributor

mdxs commented Oct 17, 2024

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
mdxs reviewed Oct 17, 2024
View reviewed changes
python_cli/advertiser.py Outdated
args = aparse.parse_args()

global hw
hw = SniffleHW(args.serport)
hw = SniffleHW(args.serport,baudrate=args.baudrate)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solved.

mdxs
mdxs reviewed Oct 17, 2024
View reviewed changes
python_cli/scanner.py Outdated
@@ -60,7 +61,7 @@ def main():
args = aparse.parse_args()

global hw
hw = make_sniffle_hw(args.serport)
hw = make_sniffle_hw(serport=args.serport,baudrate=args.baudrate)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solved.

@mdxs
Copy link
Contributor

mdxs commented Oct 17, 2024

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
Copy link
Author

bkerler commented Oct 17, 2024

@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
Copy link
Author

bkerler commented Oct 17, 2024

@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
mdxs reviewed Oct 17, 2024
View reviewed changes
README.md Outdated
@@ -244,6 +245,8 @@ options:
-d, --decode Decode advertising data
-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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed that accordingly.

@mdxs
Copy link
Contributor

mdxs commented Oct 17, 2024

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
Copy link
Contributor

mdxs commented Oct 17, 2024

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
Copy link
Contributor

mdxs commented Oct 17, 2024

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.

@mdxs mdxs mentioned this pull request Oct 17, 2024
Open
XenoKovah
XenoKovah reviewed Oct 17, 2024
View reviewed changes
python_cli/sniffle/sniffle_hw.py
baud = 921600
elif is_cp2102(serport):
baud = 921600
if baudrate is None:
baud = 921600
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdxs mdxs mdxs left review comments

@XenoKovah XenoKovah XenoKovah left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bkerler @mdxs @XenoKovah