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 PhyWhisperer backend. #22

Merged
merged 2 commits into from Oct 20, 2019
Merged

Conversation

jpcrypt
Copy link
Contributor

@jpcrypt jpcrypt commented Oct 8, 2019

No description provided.

Copy link
Contributor

@ktemkin ktemkin left a comment

Choose a reason for hiding this comment

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

This code seems good (nice and minimal, too). In testing, I was able to get short captures working, but I haven't had luck with --size 0 / continuous captures. I suspect the relevant changes to get that working well will need to be in phywhisperer, though, and not here.

Either way, there's at least one blocking thing that needs to be fixed (the import issue mentioned in comments) before this can be merged; and few other comments that you might want to address. Should be a quick merge after that. :)

viewsb/backends/phywhisperer.py Show resolved Hide resolved
viewsb/backends/phywhisperer.py Outdated Show resolved Hide resolved
viewsb/backends/phywhisperer.py Outdated Show resolved Hide resolved
viewsb/backends/phywhisperer.py Outdated Show resolved Hide resolved
@ktemkin ktemkin assigned ktemkin and jpcrypt and unassigned ktemkin Oct 8, 2019
@ktemkin ktemkin added the enhancement potential new feature label Oct 8, 2019
@jpcrypt
Copy link
Contributor Author

jpcrypt commented Oct 9, 2019

Thanks for the review! Yeah size=0 is work-in-progress on the PhyWhisperer side, it's intended for stream capture which isn't working yet.

@jpcrypt jpcrypt requested a review from ktemkin October 11, 2019 02:51
@ktemkin
Copy link
Contributor

ktemkin commented Oct 20, 2019

Finally getting around to testing the updated changes; sorry for the delay. :)

@ktemkin ktemkin merged commit f755ce1 into greatscottgadgets:master Oct 20, 2019
@ktemkin
Copy link
Contributor

ktemkin commented Oct 20, 2019

This seems to work; so merging it as a baseline. :)

Thanks!

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

Successfully merging this pull request may close these issues.

None yet

2 participants