On 26/1/21 5:34 am, Gerhard Sittig wrote:
On Sun, 2021-01-24 at 13:51 +1100, Peter Skarpetis wrote:
On 23/1/21 10:52 pm, Gerhard Sittig wrote:
On Sat, 2021-01-23 at 08:43 +1100, Peter Skarpetis wrote:
I have recently added support for the MHINSTEK MHS-5200A function
generator to libsigrok and have created a pull request for you to
merge.
https://github.com/sigrokproject/libsigrok/pull/113
I have tested it with an MHS-5225A but should work with other models
as well.
That's not mergable in its current form. Can you do some cleanup?
Introduce the files with the proper names from the start. Move
the device communication to protocol.c and keep api.c as they are
for all other drivers. Fixup style issues. Consider squashing the
parts because: From the project's perspective this is the
introduction of an initial implementation of support for this
device (the first working version which covers a basic set of
features or all those features which translate to sigrok
infrastructure). Doesn't matter how many steps it took to get
there during internal development at your place. Consider the
perspective of those who integrate your contribution. Unless the
commits' activities really are separate things from a maintenance
perspective.
I have cleaned up the code as requested and have created a new
patchset that you can access using the following link. This is a
single commit as requested.
https://github.com/peterska/libsigrok/commit/3c4ca8764a847025d624ddb2d5ac7d6896b3dc49.patch
Umm, that is ... not what I meant. Was referring to this (v1):
$ git logor master..
0f144cea5421 Initial skeleton for the MHINSTEK MHS-5200A function generator
63b1595ca97b MHINSTEK MHS-5200A driver now reads all the values from the
function generator.
910913e0b970 MHINSTEK MHS-5200A driver all the set functions now work.
6f9a2dbd5fb5 MHINSTEK MHS-5200A driver - removed mhinstek_ prefix from
functions and structures. Makes the code more readable
2f2688a263e9 MHINSTEK MHS-5200A driver - process measurements from frequency
counter
8bdf52364452 MHINSTEK MHS-5200A driver - do not set gate time befire
acquisition start.
b0e01cb129b2 Renamed driver to mhinstek-mhs-5200a for consistency with other
driver names in libsigrok
The separation of new-driver output (skeleton) and another commit
with "the flesh" _is_ very useful, both for initial review and
for subsequent maintenance. But the multitude of commits after
the skeleton to get to the first working version and initial
submission is not needed. Thought it would be obvious and
consistent with all other available public information. Yet I can
see how my response was terse.
And maybe I misread the "renamed driver" in the last commit,
assumed that filenames had changed. Haven't looked at the
details, only had a cursory glance at the set when I noticed
it could benefit from squashes.
Often times I wish more users and peer developers would help
review stuff, instead of relying on a few persons to find the
time to do all of it. Or even kling to one individual, and wait,
and wait, and ... :-/ Another option that many submitters seem to
miss is to look around, and see what was done before. VCS history
is a useful source of information, there really should not be the
need to reiterate this for every single submission again and
again and again. Gets boring really quickly, and isn't fun either
for any of the involved persons ... (This is not in person to
you, Peter, just a plea to all readers to help the project when
they can. And I feel that using public information _is_ something
that every person could do. Doesn't take a developer to answer
FAQs, or repeat what was written before a hundred times.)
Your comments were taken on-board and I have looked through other driver
submissions and have reworked my patch so that it meets the requirements
you mentioned in your emails. I have submitted a new pull request
https://github.com/sigrokproject/libsigrok/pull/115 github automatically
closed the old one when I hard reset the commits in the branch.
Bonus points for using branches, and separate branches for
several submissions. :) That's often not the case, and makes
review an unnecessary tedium. And forced pushes would be as
annoying.
Thanks.
Peter Skarpetis
virtually yours
Gerhard Sittig
--
If you don't understand or are scared by any of the above
ask your parents or an adult to help you.
_______________________________________________
sigrok-devel mailing list
sigrok-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sigrok-devel
_______________________________________________
sigrok-devel mailing list
sigrok-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sigrok-devel
