Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

optimizations and some fault tolerance #54

Merged

Conversation

JJL772
Copy link
Member

@JJL772 JJL772 commented Aug 15, 2023

Closes #53

  • Don't poll the device if E-Bus is in an error state. The device will reject all reads/writes to the PDO address space when this is the case.
  • Only read terminal IDs once, cache them for later

Tested on bhc-tst-ek9000

JJL772 added 2 commits August 16, 2023 16:01
Ends up causing problems with some of our macros. We null check device in the LOG_XXX macros, in one case we use `this` as the device pointer

The compiler then complains about comparing this with nullptr, which is assumed to always be true (this assumption is fine, we can ignore the warning)
@JJL772 JJL772 force-pushed the enh_optimizations_and_fault_tolerance branch from c9da01e to c499d9f Compare August 16, 2023 23:01
@JJL772 JJL772 force-pushed the enh_optimizations_and_fault_tolerance branch from c499d9f to 0491dd5 Compare August 16, 2023 23:02
@JJL772 JJL772 marked this pull request as ready for review August 16, 2023 23:03
@JJL772 JJL772 requested a review from klauer August 16, 2023 23:03
Copy link
Collaborator

@klauer klauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me - just a couple minor suggestions

# RELEASE.$(EPICS_HOST_ARCH).$(T_A)
#
# This file is parsed by both GNUmake and an EPICS Perl script,
# so it may ONLY contain definititions of paths to other support
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# so it may ONLY contain definititions of paths to other support
# so it may ONLY contain definitions of paths to other support

It may be in our other RELEASE files but at least ek9k can spell it right 😁

# or
# FOO = /Full/Path/To/Development/Version
# ==========================================================
ASYN=/reg/g/pcds/epics/R7.0.2-2.0/modules/asyn/$(ASYN_MODULE_VERSION)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ASYN=/reg/g/pcds/epics/R7.0.2-2.0/modules/asyn/$(ASYN_MODULE_VERSION)
ASYN=$(EPICS_MODULES)/asyn/$(ASYN_MODULE_VERSION)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!!!

@@ -218,6 +220,10 @@ class devEK9000 : public drvModbusAsyn {
/* Buffer for status info */
uint16_t m_status_buf[EK9000_STATUS_END - EK9000_STATUS_START + 1];

/* Cache of terminal layout */
uint16_t m_terminals[0xFF];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic number 0xff - yes, a well known one to be fair - should probably be a const/define somewhere?


memset(m_terminals, 0, sizeof(m_terminals));
for (int off = 0; off < 255; off += 125) {
if (off > 0 && m_terminals[off] == 0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to understand the logic of this loop...
So in iterations 2 and 3, the final element was written by the previous iteration (1 and 2) of the loop, and the final element should have something in it? What is that final element representing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Input register address 0x6000 to 0x60FF contain a numerical ID for each terminal. 0x6000 will always contain 9000 for ek9000. If the next terminal is EL4004, 0x6001 contains 4004 (in decimal), and so on. The rest of the registers are set to 0, so if the last element read is 0, there are no more terminals on the rail and we can stop reading.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. Seems worthwhile to toss in a comment to that effect 👍

@@ -11,6 +11,7 @@ ek9000Test_registerRecordDeviceDriver pdbbase

# Configure the device (EK9K_NAME, IP, PORT, TERMINAL_COUNT)
ek9000Configure("${EK9K}", "${IP}", ${PORT}, ${NUM_TERMS})
asynSetTraceMask("${EK9K}", 0, 0x21)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's 0x21 again? Probably good to mark it in a comment as ASYN_TRACE_WARNING (0x20) |ASYN_TRACE_ERROR (0x01)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea

@JJL772 JJL772 force-pushed the enh_optimizations_and_fault_tolerance branch from dce6720 to aeeba6f Compare August 17, 2023 23:02
@klauer
Copy link
Collaborator

klauer commented Aug 18, 2023

Post-review tweaks LGTM - merge at will

@JJL772 JJL772 merged commit dab0d37 into slac-epics:master Aug 18, 2023
@JJL772 JJL772 deleted the enh_optimizations_and_fault_tolerance branch August 18, 2023 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better handle device faults
2 participants