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.)
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.
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
