Pingdriver #1111
Pingdriver #1111
Conversation
Williangalvani
force-pushed
the
pingdriver
branch
7 times, most recently
from
015ce4c to
1eea259
Compare
Williangalvani
requested review from
patrickelectric and
rafaellehmkuhl
and removed request for
patrickelectric
Williangalvani
force-pushed
the
pingdriver
branch
2 times, most recently
from
91b0065 to
5643401
Compare
There was a problem hiding this comment.
New UI is great!
Love the images that show where things are plugged in, and it'll be great to have Ping1D MAVLink messages again (one step closer to full Companion replacement). Also really cool that it detects Ping360s over ethernet. From a support standpoint this update should be super helpful - thanks! :-)
No bugs found, just some minor improvements and consideration points :-)
Williangalvani
force-pushed
the
pingdriver
branch
from
8b171d8 to
39ce58d
Compare
| @@ -40,6 +40,14 @@ def get_sensors() -> Any: | |||
| return [PingDeviceDescriptorModel.from_descriptor(device) for device in ping_manager.devices()] | |||
|
|
|||
|
|
|||
| @app.post("/sensors", status_code=status.HTTP_200_OK, summary="Set sensor settings.") | |||
| @version(1, 0) | |||
| def set_sensor(sensor_settings: dict[str, Any]) -> Any: | |||
There was a problem hiding this comment.
Can't we enforce a format for the post message ? Like we have in the others services.
There was a problem hiding this comment.
the goal here is to have a somewhat abstract interface we can use for all sensors eventually, the goal is to have simple key:values for now, but allowing at least the basic types
| @app.post("/sensors", status_code=status.HTTP_200_OK, summary="Set sensor settings.") | ||
| @version(1, 0) | ||
| def set_sensor(sensor_settings: dict[str, Any]) -> Any: | ||
| if "port" not in sensor_settings: |
There was a problem hiding this comment.
This should be not necessary if we enforce the post type.
core/services/ping/pingmanager.py
Outdated
| @@ -27,15 +27,27 @@ async def register_ethernet_ping360(self, ping: PingDeviceDescriptor) -> None: | |||
| if ping not in self.drivers: | |||
| self.drivers[ping] = Ping360Driver(ping, -1) | |||
|
|
|||
| def find_next_port(self, base_port: int, direction: int) -> int: | |||
There was a problem hiding this comment.
Why not using port 0 to bind and get the port that the OS found for us ?
There was a problem hiding this comment.
I prefer following some kind of order for now...
There was a problem hiding this comment.
also, we're starting at the ports checked by pingviewer for now. we still dont´t have the discovery protocol implemented for alternative ports
Williangalvani
force-pushed
the
pingdriver
branch
7 times, most recently
from
3e9da38 to
05f4efa
Compare
Williangalvani
requested review from
ES-Alexander and
patrickelectric
and removed request for
patrickelectric
| board_type() : BoardType { | ||
| /* Detects board type between navigator and Rpi4 */ | ||
| // TODO: make sure this works with pi3 as well | ||
| const serial_port_path = this.serial_port_path as string // why is it needed? |
There was a problem hiding this comment.
@rafaellehmkuhl do you know what the cast is necessary ?
There was a problem hiding this comment.
Apparently because in line 66 it can return undefined if port is not defined.
core/services/ping/ping1d_mavlink.py
Outdated
| if exception.errno == errno.EAGAIN: | ||
| continue |
There was a problem hiding this comment.
I'm inclined to agree with @ES-Alexander here, why EAGAIN is special over others that needs his special continue ?
| accordingly to direction | ||
| """ | ||
| port = base_port | ||
| while udp_port_is_in_use(port): |
There was a problem hiding this comment.
We should limit the range that the port can be.
At least between 9090 and 9099, otherwise we could start having numbers totally outside the range if 3rd party programs are using it, without us having control to find the sensors with ping-viewer.
We should also start thinking about how ping-viewer is going to discover this devices, how that works currently ?
There was a problem hiding this comment.
I don't see the point in limiting it. Discovery needs to be improved in PingViewer.
As is it will work fine for one Ping1D and one Ping360.
We could either use the Ping360 discovery protocol, or ask Beacon to broadcast the devices using mDNS
There was a problem hiding this comment.
Not limiting the range will result in a silent loop block where there is no condition to exit besides the OS saying that a port is available, if no port is available, this loop will never exit. But if you don't have strong feelings that such thing will never happen I'm ok with it.
| board_type() : BoardType { | ||
| /* Detects board type between navigator and Rpi4 */ | ||
| // TODO: make sure this works with pi3 as well | ||
| const serial_port_path = this.serial_port_path as string // why is it needed? |
There was a problem hiding this comment.
Apparently because in line 66 it can return undefined if port is not defined.
…deal with pydantic 1.10.0a2
Williangalvani
force-pushed
the
pingdriver
branch
from
5d7ef9c to
1ba1a23
Compare
| def list_ips() -> Set[str]: | ||
| """ | ||
| Returns a list of IPs found on ethernet interfaces that are currently up. |
There was a problem hiding this comment.
| def list_ips() -> Set[str]: | |
| """ | |
| Returns a list of IPs found on ethernet interfaces that are currently up. | |
| def get_available_ips() -> Set[str]: | |
| """ | |
| Returns a set of IPs found on ethernet interfaces that are currently up. |
image at williangalvani/blueos-docker:pingdriver
what works:
Helps #264