Tracking didn't work immediately for me, and digging into a little deeper it looks like quite a few changes have occurred with Servo.  So first we should review what is currently in Servo.

I have listed out every method, its parameters and return type and sorted it - there is a comment section, it is fully editable. - the link in fullscreen view is here

Mats

7 years 4 months ago

The onRegistered method was added as a helper method for the GUI's. I found that refreshing the list of attachable controllers to the service, works well for both the Swing GUI and the WebGui, In the WebGui, populating the dropdown list becomes a simple data binding. And since it works well for the WebGui, I also used the same method for the Swing GUI, I don't remember adding it to the Servo GUI, but it is the way I built all i2c related GUI's. Makes both the SwingGUI and WebGui clean and simple. I think there currently is a bit to much code in both Sevo gui's. Cleaning up the Servo service is a very good start. 

GroG

7 years 4 months ago

Seems like a good idea Mats, to refresh the set of "attachable" service when a new service is registered. Making it work in webui in the same way as swing is always a goal and benefit when it happens.  Thanks Mats !

 

GroG

7 years 4 months ago

Hi Mats,

could you tell me if this looks ok ?   There were many unimplemented methods, so I took my best guess

https://github.com/MyRobotLab/myrobotlab/blob/develop/src/org/myrobotlab/i2c/I2CBus.java 

I find it interesting this implements Attachable but is not a Service .. I usually implement attach & detach methods which have instance references with logic rather than there string parameter counterparts.

e.g.  attach(I2CController instance) {  ...  logic for attaching }  vs the attach(String serviceName)
But it seems you had already begun with attach(String serviceName) so I just had the instance ones call the string parameters and left your logic there..

 

Hi Grog.

The only reason for I2CBus to implemet Attachable, is because I want to be able to create a device in MRLComm. 

  private void i2cBusAttach(I2CBusControl control, int busAddress) {
    Integer deviceId = attachDevice(i2cBus, new Object[] { busAddress });
    msg.i2cBusAttach(deviceId, busAddress);
  }
 
  synchronized private Integer attachDevice(Attachable device, Object[] attachConfig) {
    DeviceMapping map = new DeviceMapping(device, attachConfig);
    map.setId(nextDeviceId);
    deviceList.put(device.getName(), map);
    deviceIndex.put(nextDeviceId, map);
    ++nextDeviceId;
    return map.getId();
  }
 
I would prefer to not having I2CBus implementing Attachable, but attachDevice requires an object that implements Attachable.
 
 

GroG

7 years 4 months ago

This is what happens when I start a simple Servo + Arduino ... it disables itself in a short amount of time and does not re-enable itself.   ... :(  .. not good
 
16:56:08.697 [servo01] INFO  class org.myrobotlab.service.Servo - servo01 is disable, discarding moveTo()
16:56:08.710 [servo01] INFO  class org.myrobotlab.service.Servo - servo01 is disable, discarding moveTo()
16:56:08.751 [servo01] INFO  class org.myrobotlab.service.Servo - servo01 is disable, discarding moveTo()
16:56:08.874 [servo01] INFO  class org.myrobotlab.service.Servo - servo01 is disable, discarding moveTo()
16:56:08.874 [servo01] INFO  class org.myrobotlab.service.Servo - enableAutoDisable : false
16:56:17.571 [Timer-6] INFO  c.myrobotlab.framework.Service - servo01.detachPin(4)
16:56:17.577 [virtual.mrlcomm] INFO  c.m.arduino.virtual.MrlServo - servo id 1->detachPin()
 
 
I hope InMoov does not depend on this broken logic...

GroG

7 years 4 months ago

This seems to be a very Arduino specific thing creeping into the ServoControl ..

https://github.com/MyRobotLab/myrobotlab/blob/develop/src/org/myrobotlab/service/Servo.java#L770-L779

It's important to keep Arduino details and solutions of issues inside the Arduino service, otherwise they can begin to interfere with other ServoControllers.

If this logic is necessary for a Servo to attach to an Arduino - it should be here :
https://github.com/MyRobotLab/myrobotlab/blob/develop/src/org/myrobotlab/service/Arduino.java#L1587

that's a whole lot of sleeping going on in that attachServoController(ServoController controller)  ...  should be replaced with attach(ServoController controller)   IMHO.

 

GroG

7 years 4 months ago

https://github.com/MyRobotLab/myrobotlab/blob/develop/src/org/myrobotlab/service/interfaces/ServoController.java#L75-L84

 

This breaks convention of {type}{action} ...
All other methods begin with servo{action} ... really they all should be the interface not the type .. ie servoControl{action} ... but at this point I would say its not worth changing

enablePin really needs to be reviewed..   if the objective is to have an analog touch sensor affect the servo we should figure out the pattern of feedback devices..   Sensor could and probably should be a feedback listener, but it should be general, nor confined to the same ServoController which the servo is controlled from.

More likely, the Servo should become something like a PinArray listener

GroG

7 years 4 months ago

enableAutoAttach (boolean autoAttach)
enableAutoDetach (boolean autoDetach)
enableAutoDisable (boolean autoDisable)
enableAutoEnable (boolean autoEnable)

This has become a bit messy...

kwatters suggested the following
setAutoEnable(boolean) & setAutoDisable(boolean)

I would like to grep & remove vs deprecate 4 methods :P  Are they really used seperately ?  If so why & how ?

grep'ing through all repos pyrobotlab, myrobotlab, and inmoov ... for all java, py, js & html - I only see the following ...

As I mentioned, I want to only have setAutoEnable(boolean) & setAutoDisable(boolean) at the most - the 4 other methods should simply be removed...

 

While you are going through all of this, is it also worth updating the documentation for the servo in the services documentation on the website?

 

I noticed that the website still lists the setSpeed function which I thought was being depreciated in favor of the setVelocity method which isn't listed at all!