jacobschaer / python-doipclient

Pure Python ISO 13400 Client
MIT License
151 stars 50 forks source link

Add classmethod get_entity to send Vehicle ID Req without TCP connect #28

Closed HarshaLaxman closed 1 year ago

HarshaLaxman commented 1 year ago

17

addr, resp = DoIPClient.get_entity()

Binds to dynamically assigned port (UDP_TEST_EQUIPMENT_REQUEST) and sends to UDP_DISCOVERY port. Send IP defaults to broadcast, with the option to send unicast by passing in a IP address (IPV4 only for now)

Created a staticmethod to create the UDP socket that is used by await_vehicle_announcement and get_entity. Since the latter calls the former after sending the request, we reuse the socket.

HarshaLaxman commented 1 year ago

Shall I also uprev the version in setup.py ?

jacobschaer commented 1 year ago

You can leave the version string alone - I can roll it and publish when this goes in. Depending on if we can keep the same function interface or not I guess will determine minor/major.

jacobschaer commented 1 year ago

Thanks for doing this! Some comments, the big thing is hoping we can keep the existing functions as-is since I imagine they're being used and make this it's own thing. If you're not confident in the IPV6 we can leave it IPV4 only until someone who has access to an IPV6 system can test.

@feuerball @MultiName90

jacobschaer commented 1 year ago

Looks like I need to fix the workflow to exclude python 3.6 now as well :( Done - you can rebase at some point to get the updated workflow files.

HarshaLaxman commented 1 year ago

And 🥂 for putting this out there! :)

jacobschaer commented 1 year ago

A quick glance, the spec just calls both use cases "vehicle discovery" and I don't see any particular name for this other than it's the "alt vehicle discovery" scenario lol.

NI calls it "doip get entities " it looks like. I couldn't find a reference in autosar. So maybe just broadcast_vehicle_discovery or get_entities or similar?

For a future enhancement, these two discovery functions probably should have a version that blocks for a specified amount of time and returns a collection since there could be more than one ECU. But will need some thought and not really important here, just musing.

HarshaLaxman commented 1 year ago

Renamed to get_entity and removed IPV6 and the related source_interface option.

Brought back the original request_vehicle_identification

And rebased to get the updated workflow

And agree 💯 re the future enhancement!

jacobschaer commented 1 year ago

Looks good and unit tests pass. Last couple things:

Can you add the new use case to the doc/source/index.rst ? Right now there's an example using the await_vehicle_announcement() workflow. If you could add a new snippet there that would be great for the readthedocs page.

.. code-block:: python

    from doipclient import DoIPClient
    address, announcement = DoIPClient.get_entity()
    logical_address = announcement.logical_address
    ip, port = address
    print(ip, port, logical_address)

Otherwise looks good to merge.

HarshaLaxman commented 1 year ago

Added the use case to index.rst !

jacobschaer commented 1 year ago

Merged, thanks for the help