(POC) feat: can aux #432

IliyanKordev · 2023-12-14T14:36:59Z

No description provided.

codecov · 2023-12-14T14:39:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (cc9690d) 98.11% compared to head (74209ae) 98.16%.
Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #432      +/-   ##
==========================================
+ Coverage   98.11%   98.16%   +0.04%     
==========================================
  Files          80       83       +3     
  Lines        6487     6637     +150     
==========================================
+ Hits         6365     6515     +150     
  Misses        122      122

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sebclrsn · 2023-12-18T10:39:53Z

src/pykiso/lib/auxiliaries/can_auxiliary.py

+        msg_to_send = self.parser.encode(message, signals)
+        self.channel.cc_send(msg_to_send[0], remote_id=msg_to_send[1])
+
+    def run_command(


Why is this API reimplemented? It's already part of the DTAuxiliaryInterface

Ok, i will remove it

sebclrsn · 2023-12-18T10:40:51Z

src/pykiso/lib/auxiliaries/can_auxiliary.py

+        return None
+
+    @staticmethod
+    def _is_payload_valid(payload: Optional[bytes]) -> bool:


This doesn't look like it belongs here

I remove it

sebclrsn · 2023-12-18T10:42:18Z

src/pykiso/lib/auxiliaries/can_auxiliary.py

+        super().__init__(
+            is_proxy_capable=True, tx_task_on=True, rx_task_on=True, **kwargs
+        )
+        self.tx_task_on = False


This should be set in the line above

sebclrsn · 2023-12-18T10:43:11Z

src/pykiso/lib/auxiliaries/can_auxiliary.py

+        self.channel = com
+        self.queue_tx = queue.Queue()
+        self.queueing_event = threading.Event()
+        self.collect_messages = functools.partial(_collect_messages, com_aux=self)


These 3 attributes don't seem to be used, you could remove the _collect_messages class above too

Ok, i will remove it and test it

sebclrsn · 2023-12-19T14:27:00Z

src/pykiso/can_message.py

+    def __init__(self, name: str, signals: dict[str, Any], time_stamp: float) -> None:
+        self.name = name
+        self.signals = signals
+        self.time_stamp = time_stamp


Suggested change

self.time_stamp = time_stamp

self.timestamp = timestamp

ok, i will change it

sebclrsn · 2023-12-19T14:27:09Z

src/pykiso/can_message.py

+
+
+class CanMessage():
+    def __init__(self, name: str, signals: dict[str, Any], time_stamp: float) -> None:


Suggested change

def __init__(self, name: str, signals: dict[str, Any], time_stamp: float) -> None:

def __init__(self, name: str, signals: dict[str, Any], timestamp: float) -> None:

ok, i will change it

sebclrsn · 2023-12-19T14:28:19Z

src/pykiso/can_message.py

+
+
+class CanMessage():
+    def __init__(self, name: str, signals: dict[str, Any], time_stamp: float) -> None:


We are supposed to be compatible with python 3.7+, so using builtin types as type hints isn't possible

Use the ones from typing instead:

from typing import Any, Dict Dict[str, Any]

Ok, i will fix it

sebclrsn · 2023-12-19T14:32:35Z

src/pykiso/lib/auxiliaries/can_auxiliary.py

+                    self.can_messages[message_name].name = message_name
+                    self.can_messages[message_name].signals = message_signals
+                    self.can_messages[message_name].time_stamp = message_timestamp
+        except (Exception, KeyError):


Simply catching the Exception will already catch the KeyError, if you need a different error message you might want to handle them separately:

except KeyError: log.exception("Some message") except Exception: log.exception("My very generic message")

Ok, i will separate them

sebclrsn · 2023-12-19T14:39:54Z

src/pykiso/lib/auxiliaries/can_auxiliary.py

+        :return: list of last can messages or None if no messages for this component
+        """
+        last_msg = self.can_messages.get(message_name, None)
+        time.sleep(timeout)


This isn't precise at all, it anyone wants to know how long it took until the message arrived he can't

Instead of having the potential message as can_messages values, you could also have a Queue with maxsize=1 which would allow you to use the non-polling APIs .get(timeout=...)

Pog3k · 2024-01-05T13:18:55Z

examples/test_can/test_can.py

+    def setUp(self):
+        pass
+
+    def test_run(self):


Normally tests have an assert statement which you don't use here..
I would suggest to reduce the example to only one extra thread.
Just start tx thread and wait for incoming message in this test method.

Do an assert on the received message where you know the signal content

Ok, i agree, i will fix it

Pog3k · 2024-01-05T13:21:41Z

examples/test_can/test_can.py

+        recv_t.join()
+
+        send_t = threading.Thread(
+            target=self.send_message, args=[can_aux1, messages_to_send, 0.2]


you could simplify this. Just pass the can_aux1.

Suggested change

target=self.send_message, args=[can_aux1, messages_to_send, 0.2]

target=tx_thread, args=[can_aux1]

You could also define the tx_thread function inside this mehtod.
Everything what you have passed can be defined in the new tx_thread function

ok, but method will be the same in two tests. Is it more appropriate to define it once in the class

Pog3k · 2024-01-05T13:23:07Z

examples/test_can/test_can.py

+        send_t = threading.Thread(
+            target=self.send_message, args=[can_aux1, messages_to_send, 0.2]
+        )
+        recv_t = threading.Thread(


don't start an extra receive thread.
Just receive the message in this method an do an assert on the expected message

Pog3k · 2024-01-05T13:24:52Z

examples/test_can/test_can.py

+            target=self.get_last_msg_and_wait_for_new_msg, args=[can_aux2, 1]
+        )
+
+        send_t.start()


I think here is a good point to create another test.

def test_wait_for_expected_signals(self):

Ok, i will separate them

Pog3k · 2024-01-05T13:26:52Z

examples/test_can/test_can.py

+    def setUp(self):
+        pass
+
+    def test_run(self):


Every method which starts with "test"_ is considered to be run as a test from the unittest framework
I think you could split the test_run into 2 tests
Name the first

Suggested change

def test_run(self):

def test_send_and_receive_message(self):

and the second one
def test_wait_for_expected_signals(self):

Every test method has at least one assert statement

Ok, i will separate them

Pog3k · 2024-01-05T13:27:44Z

examples/test_can/test_can.py

+    def tearDown(self):
+        pass
+
+    def send_message(self, can_aux, messages_to_send, time_between_messages):


Suggested change

def send_message(self, can_aux, messages_to_send, time_between_messages):

def tx_thread(self, can_aux):

define messages_to_send, time_between_messages inside this method

ok, i agree

Pog3k · 2024-01-05T13:28:53Z

examples/test_can/test_can.py

+            can_aux.send_message(message_to_send.name, message_to_send.signals)
+            time.sleep(time_between_messages)
+
+    def get_last_msg_and_wait_for_new_msg(self, can_aux, timeout):


a better name would have rx_thread but as suggested just put the content in your test method which is currently "test_run"

Pog3k · 2024-01-05T13:32:39Z

examples/test_can/test_can.py

+            "New msg with wait Signal A = " + str(recv_msg.signals["signal_a"])
+        )
+
+    def wait_for_match_message_with_signals(self, can_aux, expected_signals, timeout):


method not necessary. Just use the code here and put it into the suggested second test method .
def test_wait_for_expected_signals(self):

sebastianpfischer · 2024-02-07T14:52:37Z

Realise #373
Will be refactored with #452

IliyanKordev and others added 3 commits December 11, 2023 15:56

Update dummy.yaml

16b3881

Merge branch 'eclipse:master' into feature/can_aux

a38b5f0

Add Can Auxiliary

c09453c

sebclrsn requested changes Dec 18, 2023

View reviewed changes

sebclrsn requested review from Pog3k and TedRio December 18, 2023 10:43

Remove unnecessary attributes and methods

96e8639

IliyanKordev requested a review from sebclrsn December 19, 2023 13:14

sebclrsn requested changes Dec 19, 2023

View reviewed changes

IliyanKordev added 12 commits January 2, 2024 10:57

Fix comments and refactor the code

0edb7b2

Fix timings in can example

c293db0

Add queue for can msg and refactor the code

26ab353

Format the code

55cd1e6

Uncomment logs and fix naming

3aff92d

Add unit tests and fix bugs int the code

e491260

fix method docstring

7a6bbaf

Fix tests

3481479

Fix failing tests

8bc19f4

Add cantools in poetry and format the code

5891b70

Add can_aux examples

d52bc6e

Add test with get signal with empty queue

6e18b1f

Pog3k reviewed Jan 5, 2024

View reviewed changes

IliyanKordev added 9 commits January 5, 2024 16:27

Apply suggestions and refactor the code

2bfee29

Add tests for send message

af7b86f

Fix wait for signal test

2fd5a45

Add test for decode parser

e0ff559

Add tests for parser

d76fc47

Add run command tests

5766e2e

Remove pccan from PR

9b6374f

Add docs

fda36ec

Add tests for create and delete aux

74209ae

sebclrsn approved these changes Jan 9, 2024

View reviewed changes

Pog3k approved these changes Jan 9, 2024

View reviewed changes

Pog3k merged commit afa65e8 into eclipse-kiso-testing:master Jan 10, 2024
5 checks passed

Pog3k linked an issue Jan 24, 2024 that may be closed by this pull request

CanAuxiliary Read/Write base on dbc file #424

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(POC) feat: can aux #432

(POC) feat: can aux #432

IliyanKordev commented Dec 14, 2023

codecov bot commented Dec 14, 2023 •

edited

Loading

sebclrsn Dec 18, 2023

IliyanKordev Dec 19, 2023

sebclrsn Dec 18, 2023

IliyanKordev Dec 19, 2023

sebclrsn Dec 18, 2023

IliyanKordev Dec 19, 2023

sebclrsn Dec 18, 2023

IliyanKordev Dec 19, 2023

sebclrsn Dec 19, 2023

IliyanKordev Jan 2, 2024

sebclrsn Dec 19, 2023

IliyanKordev Jan 2, 2024

sebclrsn Dec 19, 2023

IliyanKordev Jan 2, 2024

sebclrsn Dec 19, 2023

IliyanKordev Dec 19, 2023

sebclrsn Dec 19, 2023

Pog3k Jan 5, 2024

IliyanKordev Jan 5, 2024

Pog3k Jan 5, 2024

IliyanKordev Jan 5, 2024 •

edited

Loading

Pog3k Jan 5, 2024

IliyanKordev Jan 5, 2024

Pog3k Jan 5, 2024

IliyanKordev Jan 5, 2024

Pog3k Jan 5, 2024 •

edited

Loading

IliyanKordev Jan 5, 2024

Pog3k Jan 5, 2024

IliyanKordev Jan 5, 2024

Pog3k Jan 5, 2024

IliyanKordev Jan 5, 2024

Pog3k Jan 5, 2024

sebastianpfischer commented Feb 7, 2024



		class CanMessage():
		def __init__(self, name: str, signals: dict[str, Any], time_stamp: float) -> None:

	target=self.send_message, args=[can_aux1, messages_to_send, 0.2]
	target=tx_thread, args=[can_aux1]

	def send_message(self, can_aux, messages_to_send, time_between_messages):
	def tx_thread(self, can_aux):

(POC) feat: can aux #432

(POC) feat: can aux #432

Conversation

IliyanKordev commented Dec 14, 2023

codecov bot commented Dec 14, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IliyanKordev Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pog3k Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastianpfischer commented Feb 7, 2024

codecov bot commented Dec 14, 2023 •

edited

Loading

IliyanKordev Jan 5, 2024 •

edited

Loading

Pog3k Jan 5, 2024 •

edited

Loading