Prestonr feature add wkc driver saves #110

preston-rogers · 2024-07-31T01:52:33Z

Added a function that can be called by fastcat to confirm if jsd devices are operational at time of bad working counter. This will allow one to differentiate between situations where the CPU is overloaded and if there is simply poor physical connections to a device.

I have attached three pictures:
The first being where the entire JSD device is turned off (corresponds to poor power to the jsd bus).
The second where a single device at the end of the bus was unplugged (corresponds to a single bad device connection).
The third being where the timeout for jsd_read was set to a very small number to force the underlying SOEM function to time out (corresponds to no faulty JSD connections).

TODO:
Need to update the jsd tests to avoid test failures.

alex-brinkman

Good work, this feature will definitely help diagnose and debug issues. checkout my review comments and feel free to reach out for more discussion

CMakeLists.txt

src/jsd.c

At Anduril

kwehage

See comments

kwehage · 2024-08-29T21:07:51Z

src/jsd.c

@@ -226,6 +228,62 @@ bool jsd_init(jsd_t* self, const char* ifname, uint8_t enable_autorecovery) {
  return true;
 }

+bool jsd_all_slaves_operational(jsd_t* self) {
+  int slave;


Reduce the scope of slave here:

for(int slave=1, slave <= *self.->ecx_context.slavecount; slave++) { ... }

It's also weird that they're using a signed integer to refer to a count, but this comes from SOEM

kwehage · 2024-08-29T21:14:27Z

src/jsd.c

+  else {
+    MSG("Some slaves were not operational.");
+    if (self->ecx_context.ecaterror) {
+      MSG("We experienced an ECAT error. When this occurs, error information aught to be saved. "


aught is not the correct word here. Who is "we"? Please update error message as:
"An ECAT error occurred; the error list is displayed below:"

kwehage · 2024-08-29T21:15:06Z

src/jsd.c

+    MSG("Some slaves were not operational.");
+    if (self->ecx_context.ecaterror) {
+      MSG("We experienced an ECAT error. When this occurs, error information aught to be saved. "
+          "Errors in error list displayed below:\n");


Do we need to add a line break here? doesnt this add an extra line of whitespace?

Correct, the MSG macro already adds a newline, so no need to add one manually

Modified this

kwehage · 2024-08-29T21:16:16Z

src/jsd.c

+    if (self->ecx_context.ecaterror) {
+      MSG("We experienced an ECAT error. When this occurs, error information aught to be saved. "
+          "Errors in error list displayed below:\n");
+      while(self->ecx_context.ecaterror) MSG("%s\n", ecx_elist2string(&self->ecx_context));


This needs a timeout or counter to avoid the (unlikely) possibility of polling indefinitely.

Welp, I was running this with srl-gcat and I ended up with an infinite loop, I had to ctrl+\ to exit.

And FWIW, my infinite loop was just spamming blank lines, (i.e ecx_elist2string(&self->ecx_context) == "")

Oh unpacking this some more self->ecx_context.ecaterror is a bool, so unless we are going to clear that error flag, this is meant to "block" the application, and not really a way to unfurl via ecx_elist2string all the possible errors in the error list.
If there's only one error, we would just print it over and over. I am not sure if this is the intent/outcome we want

Actually this is a bug that Will and I found with SOEM. Even though the elist is empty, ecaterror still states there is an error. The ecaterror flag aught to be cleared once the list is emptied.
So @kwehage fear of an infinite loop is valid

The ecaterror flag aught to be cleared once the list is emptied.

It's more low level, but do you want to consider this as the conditional?
https://github.com/nasa-jpl/jsd/pull/112/files#diff-ec8bc2dbf8b89c6e78c73f1adf5732f1dff61d11f7cf4f22dd01241cb7cb39e3R285

@kwehage I just added a counter that once a certain max count is reached, it terminates the while loop

kwehage · 2024-08-29T21:18:00Z

src/jsd.c

+      MSG("We experienced an ECAT error. When this occurs, error information aught to be saved. "
+          "Errors in error list displayed below:\n");
+      while(self->ecx_context.ecaterror) MSG("%s\n", ecx_elist2string(&self->ecx_context));
+      MSG("Went through all errors in the elist stack.\n");


If you call it an "error list" above, call it an "error list" here also. Is it a stack or a list? Better to change to something like:
"Finished reporting errors in error list" or just remove this line altogether.

It's a list of errors implemented as a stack. I'll change the message, however

kwehage · 2024-08-29T21:19:17Z

src/jsd.c

+    if (self->ecx_context.slavelist[slave].state != EC_STATE_OPERATIONAL) {
+      all_slaves_operational = false;
+      if (self->ecx_context.slavelist[slave].state ==
+          (EC_STATE_SAFE_OP + EC_STATE_ERROR)) {


I wondered about this weird syntax, but I see it comes from SOEM, so all is good.

Yeah Dennis commented on this too. If they're both one-hot pieces of data, an addition does the same as a bitwise OR

preston-rogers added 10 commits July 29, 2024 16:50

Adding the wkc

55f9df1

Making one last change

c77bcb9

Adding the stuff

f06e8cf

One bug fix

b3d0840

last fix

c8d26e9

last fix

5339af5

Adding stuff to avoid malloc

03c7b36

Adding inspect context to be used at time of jsd read

1cdfe4f

Individual checks

b7e4462

fix typo

7bacf19

preston-rogers requested review from JosephBowkett, kwehage and alex-brinkman July 31, 2024 01:52

preston-rogers self-assigned this Jul 31, 2024

preston-rogers requested a review from a team as a code owner July 31, 2024 01:52

preston-rogers added 2 commits July 31, 2024 08:13

Fixing the build issues

465c767

Added one line to see which devices are not being registered properly

1c3af6a

alex-brinkman previously requested changes Aug 2, 2024

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

src/jsd.c Outdated Show resolved Hide resolved

preston-rogers added 8 commits August 5, 2024 10:29

minor message change

4ef43e2

Removing bad wkc stuff

dbe2702

Removing line

650cf9a

One more time

4ba6532

Minor msg change

59e7cf5

Making changes to jsd_inspect_context

8c966f1

fix

d11d65c

fix 2

b0cae7c

preston-rogers requested a review from alex-brinkman August 13, 2024 23:19

kwehage requested changes Aug 29, 2024

View reviewed changes

Adding a max elist display counter

2e121af

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prestonr feature add wkc driver saves #110

Prestonr feature add wkc driver saves #110

preston-rogers commented Jul 31, 2024 •

edited

Loading

alex-brinkman left a comment

kwehage left a comment

kwehage Aug 29, 2024

kwehage Aug 29, 2024

kwehage Aug 29, 2024

preston-rogers Aug 30, 2024

kwehage Aug 29, 2024

dwai-wai Aug 29, 2024

preston-rogers Aug 30, 2024

kwehage Aug 29, 2024

dwai-wai Aug 29, 2024

dwai-wai Aug 29, 2024

preston-rogers Aug 30, 2024

dwai-wai Aug 30, 2024

preston-rogers Aug 30, 2024

kwehage Aug 29, 2024

preston-rogers Aug 29, 2024

kwehage Aug 29, 2024

preston-rogers Aug 30, 2024

Prestonr feature add wkc driver saves #110

Are you sure you want to change the base?

Prestonr feature add wkc driver saves #110

Conversation

preston-rogers commented Jul 31, 2024 • edited Loading

alex-brinkman left a comment

Choose a reason for hiding this comment

kwehage left a 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

preston-rogers commented Jul 31, 2024 •

edited

Loading