Closed e-rk closed 5 years ago
We should not merge this until https://github.com/NordicPlayground/nRF-802.15.4-sniffer/pull/19 is finished.
One doubt: version 1.0 without any regression tests? Shouldn't be 0.1?
@hubertmis I think it's up to a debate. One can for example treat feature-completeness as the criteria for 1.0 release. IMO the sniffer as it is now is almost complete from the user standpoint except for the toolbar channel selection.
I have manually tested the operation of the extcap on both Windows and Ubuntu, with Python 2.7 and 3.7 on both Wireshark 2.6.7 and 3.0.0 versions. The only fix remaining that has to make it's way to master is here as it caused the newline character to be undetected on Python 3.7.
@e-rk , It's not up to debate. Have a look at 'Naming and version numbering' page in Confluence. Feature-completeness is the criteria for 0.1 release. Product maturity is the criteria for 1.0 release.
It's up to to debate if this project is mature. I think regression tests is one of product maturity criteria. I have some more concerns about maturity of this project, we can discuss F2F :)
@stig-bjorlykke Thanks a lot for the heads up. This could be even made the default behaviour for Wireshark 3.0.0 by checking the new --extcap-version
command line parameter. It would be really nice to have this for the release and I will appreciate the PR a lot.
@hubertmis Thanks for explaining and giving me some pointers. I'll be glad to discuss more improvements with you. Consider me convinced on the version number.
Actually the new --extcap-version
parameter is only implemented for a specific extcap feature, namely listing interfaces, and is not used when starting a capture. I would consider this a Wireshark bug.
The best way to solve this for the release is probably to add a setting in the Interface Options dialog to set DLT to use (without metadata, metadata on V2 using the provided Lua script and metadata on V3 using the builtin TAP support), optionally with some describing help text. Then the user can change this without editing the code.
I agree that the current version of the sniffer should not be 1.0, but mostly because of the maturity (I don't buy argument with automation tests though, although indeed this will be main priority for this project after this release).
Personally to promote this solution, I would use e.g. version 0.5.0 which in contrast to 0.1.0 does not suggest the initial stage of the project. There were many iterations already and it is successfully deployed in our regression tests. But I'm okay with following best practices.
Thanks @stig-bjorlykke we will take a look on adding DLT select in the interface configuration.
@LuDuda I have created a separate PR for configurable DLT (out-of-data meta-data) support.
After discussion, the version is changed to 0.7.0
.
Hi @greg-fer, Could you please review my changes? Thank you.
All checks done on Windows and Ubuntu with WS 2.6.7 and 3.0.0 on both Python 2.7 and 3.7. I firmly believe this is ready to merge now.
@LuDuda @stig-bjorlykke Good catch. Toolbar has been now removed.
Thanks LGTM :+1: Great work on that!
Next time remember to update extcap {version=1.0}
in extcap_interfaces()
to reflect the released version. This version is shown in the Help -> About Wireshark -> Plugins
list.
Documentation changes:
Additional extcap improvements:
Firmware changes: