DeepBlueRobotics / DeepBlueSim

MIT License
8 stars 0 forks source link

Add PoweredHingeJoint.proto. #71

Closed brettle closed 4 months ago

brettle commented 4 months ago

Also:

brettle commented 4 months ago

Fixes #51.

CoolSpy3 commented 4 months ago

About to take a look at this, but I realized the windows CI has been running for almost 3 hours. Any ideas on why that would happen? It works for me locally.

brettle commented 4 months ago

About to take a look at this, but I realized the windows CI has been running for almost 3 hours. Any ideas on why that would happen? It works for me locally.

Not off hand. Looking into it. Could it be related to your earlier CI changes?

Not sure if this is normal.

CoolSpy3 commented 4 months ago

Not off hand. Looking into it. Could it be related to your earlier CI changes?

That was my first thought as well, but I'm not 100% sure how.

Not sure if this is normal.

I think it is.

CoolSpy3 commented 4 months ago

Looking at it, it doesn't appear that the check task ever actually started. (ie. the program's still waiting for it to start up) :/

brettle commented 4 months ago

Looking at it, it doesn't appear that the check task ever actually started. (ie. the program's still waiting for it to start up) :/

The systemTest task is running so check must be running. I've downloaded a copy of the log. I'm going to 1) update the branch, and 2) add a Timeout annotation to the SystemTestRobot class. That should at least give us access to the webots log which might provide more clues.

brettle commented 4 months ago

I can't reproduce it.

  1. The ci run that started when I updated the branch succeeded on everything.
  2. The ci run that started when I added the timeout also succeeded on Windows but timed out out on macos only seconds before the test would have finished (!).
  3. The ci run that hung and I restarted succeeded on everything.
  4. The webots log always appears to be empty.

I'm going to bump the timeout up to 25 minutes in hopes of at least making the test pass on macos consistently.

brettle commented 4 months ago

I'm going to bump the timeout up to 25 minutes in hopes of at least making the test pass on macos consistently.

You beat me to it.

brettle commented 4 months ago

There's still something weird going on here with the encoder base protos.

To help you understand where I was coming from... I had thought that in PoweredHingeJoint.proto if I said:

    field MFNode{WPIMotorBase{}, WPIEncoderBase{}, Brake{}} devices [NEOMotor{} REVBuiltinEncoder{} Brake{}]

then any current or future protos that derive from WPIMotorBase or WPIEncoderBase would be allowed. I also thought I'd be able to tell in the js whether a particular encoder device was a subclass of WPIBuiltInEncoderBase so I'd know whether it was prohibited with a PWM controller. Unfortunately, Webots isn't currently smart enough to do either of those things at the moment. But, if it is in the future, the extra layers in the hierarchy will be useful. Since I'd already done the work, I figure I might as well leave them in.

In terms of how the actual encoder protos behave, I was trying to minimize changes to the actual java code.

What I think makes sense, based on the other changes, is to remove the now unused fields from WPIEncoderBase.

Done.

(This makes it the same as PositionSensor, so we could also remove the proto entirely and just use PositionSensor directly.)

I'd rather keep it as a marker interface, a place for potentially adding fields that are frc specific across all encoders, and a way of possibly easily limiting which devices are allowed in other protos if Webots gets that capability.

WPIBuiltinEncoderBase should either be exposed directly (and REVBuiltinEncoder deleted) or should accept some parameter indicating the type of builtin encoder. (Otherwise, there's no distinction between it and its subtypes.)

The type of the builtin encoder is the name of the proto. Right now there is only one subclass and it doesn't need any extra fields because the motor and its controller fully determine its details. Other subclasses might benefit from those fields.

It should then be updated to have a name of something like DBSim_BuiltinEncoder or DBSim_BuiltinEncoder_Type to remain consistent with our other protos.

I didn't do the name mangling because I didn't want to trigger the current java code to create an additional encoder mediator when it already creates one when it connects the motor. If that logic doesn't work for a future built-in encoder, it's proto can do the name mangling.

As stated in the comment, the controller will determine the other necessary parameters. They are unneeded and can be deleted.

Per above, in the future there might be additional subclasses that might need some of the other fields (e.g. the TalonFX encoder is actually an absolute encoder). WPIBuiltinEncoderBase enables that.

WPIStandalineEncoderBase already does the name calculation, so there's no need to pass the other parameters to it's super-object.

Fair enough. I've removed them.