search

indigo-astronomy / indigo

INDIGO is a system of standards and frameworks for multiplatform and distributed astronomy software development designed to scale with your needs.
http://www.indigo-astronomy.org
Other
148 stars 64 forks source link

Add support for Pegasus Pocket Powerbox Advanced #356

Closed aaronfreimark closed 4 years ago

aaronfreimark commented 4 years ago

I have a PPBA and wanted it to work :-) I created this by modifying the example in indigo_drivers/aux_ppb, and adding a very little bit of code from "Ultimate" for the power warning light.

TODO: Setting for the variable power port. It can be turned on and off here (1/0) but the PPBA also accepts a voltage in the same field. TODO: I didn't modify the simulator.

It also seems to me that the lights should show in AstroImager at the bottom of the window, but they do not show.

aaronfreimark commented 4 years ago

I noticed the light does show in AstroImager 3.x, so perhaps there was a change to AI 4.

rumengb commented 4 years ago

I have several concerns about the pull request.

  1. It contains binary file fxload which should not be there.
  2. Maybe it is a better idea to extend the current ppb driver rather than creating a new one.. Because of the first reason alone we can not accept this pull request.
rumengb commented 4 years ago

I am sorry but I have several more things i am not ok with:

  1. the pull request contains merge.
  2. the pull request contains add and then revert of some changes
  3. the driver can not be listed as stable in its original commit.
  4. Please investigate if you can distinguish PPB devices and merge your changes in the existing driver enabling and disabling features according ti the connected device. It is not ok to have several copies of the same code (driver in this case) with minor modifications for several reasons: it is hard to maintain (same bugs should be fixed in several places, enhancements should be implemented in several places too), bad user experience (it is better to load ppb driver and all ppb devices to be supported rather than loading several specific drivers), loading 2 drivers will require more resources etc. See how it is done in UPB driver it supports UPB and UPB2 devices maybe the same approach can be used.
  5. Does the simulator contain the changes to work with UPBA or it supports only the basic UPB command set? Same applys for the simulator as for the driver - one simulator (add the new commands depending on the device simulated). Here it is ok to add something like #ifdef PPBA and specify at compile time what device will be simulated.

Do not get me wrong, nothing personal :) Just tying to maintain high quality of the code and the product. So please consider those notes!

Also I would prefer to wait for Peter. To look at this too as he is the original author of upb and ppb drivers.

Thank you for your effort and we will be glad to add your UPBA support to INDIGO, it just needs several tweaks :) So please address those and we will be more than happy to accept your code :)

P.S. a quick look docs shows that command P# can be used to distinguish devices: PPBA responds with PPBA_OK while PPB reponds with PPB_OK, Please use it...

aaronfreimark commented 4 years ago

@rumengb Thank you for this feedback. I've been working in commercial software my entire career, not as a developer though. And this is my first pull request! Thank you for maintaining a product that inspired my entry into open source!

I do appreciate the constructive comments, and I'll put more effort into the code. I will submit again once it is improved.

rumengb commented 4 years ago

@aaronfreimark welcome to open source, and thank you for the nice words :) I am glad you want to contribute and we are waiting for your ppb driver update :) I am also glad you were not offended by my comments :) Some people take this quite personal and i feel uncomfortable to comment and refuse code. So do not get discouraged! Waiting for your contribution and who knows you may become a regular contributor which would be great :)

polakovic commented 4 years ago

Hi, Rumen is right, it will be MUCH easier to change indigo_aux_ppb driver to works with both standard and advanced powerbox (the same way as indigo_aux_upb driver works with both v1 and v2 UPB). I'm on a vacation until end of next week so if you want, you can try to change the driver. If you want just test it, I'll do it as soon as I'll be back :)