Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
import org.epics.vtype.Alarm;
import org.epics.vtype.AlarmSeverity;
import org.epics.vtype.AlarmStatus;
import org.epics.vtype.Display;
import org.epics.vtype.DisplayProvider;
import org.epics.vtype.Time;
import org.epics.vtype.VByteArray;
import org.epics.vtype.VEnum;
Expand All @@ -30,6 +32,7 @@
import org.phoebus.core.vtypes.VTypeHelper;
import org.phoebus.pv.PV;
import org.phoebus.pv.PVPool;
import org.phoebus.pv.PVPool.TypedName;

import io.reactivex.rxjava3.disposables.Disposable;

Expand All @@ -51,7 +54,8 @@ public class PVTableItem
private volatile VType value;

/** Value of the PV's description */
private volatile String desc_value = "";
private volatile String desc_value = null;
private volatile String desc_name = "";

/** Saved (snapshot) value */
private volatile Optional<SavedValue> saved = Optional.empty();
Expand All @@ -77,6 +81,9 @@ public class PVTableItem

/** Listener to description PV */
private volatile Disposable desc_flow;

private static final String DESC_FIELD = "DESC";
private static final String DOT = ".";

/** Initialize
*
Expand Down Expand Up @@ -122,16 +129,24 @@ private void createPVs(final String name)
updateValue(null);
return;
}
PV new_pv = null;
try
{
updateValue(VString.of("", Alarm.disconnected(), Time.now()));
final PV new_pv = PVPool.getPV(name);
new_pv = PVPool.getPV(name);
value_flow = new_pv.onValueEvent()
.throttleLatest(Settings.max_update_period_ms, TimeUnit.MILLISECONDS)
.subscribe(this::updateValue);
permission_flow = new_pv.onAccessRightsEvent()
.subscribe(writable -> listener.tableItemChanged(PVTableItem.this));
pv.set(new_pv);
// read the value for getting description
if (new_pv != null) {
VType newVal = new_pv.read();
if(newVal != null){
updateValue(newVal);
}
}
}
catch (Exception ex)
{
Expand All @@ -141,19 +156,25 @@ private void createPVs(final String name)
Time.now()));
}

// For CA PVs, check the .DESC field
// Hardcoded knowledge to avoid non-record PVs.
if (Settings.show_description &&
! (name.startsWith("sim:") ||
name.startsWith("loc:")))
{
// Determine DESC field.
// If name already includes a field,
// replace it with DESC field.
final int sep = name.lastIndexOf('.');
final String desc_name = sep >= 0
? name.substring(0, sep) + ".DESC"
: name + ".DESC";
// First try to get description from value or pv
updateDescription();
// If still no description found and channel access source
final TypedName type_name = TypedName.analyze(name);
String dataType = type_name != null ? type_name.type : null;
boolean channelAccess = dataType.equals("ca");
if (Settings.show_description && desc_value == null && channelAccess) {
// For CA PVs, check the .DESC field
// Hardcoded knowledge to avoid non-record PVs.
// First get default datasource
desc_name = name + DOT + DESC_FIELD; // by default add .DESC
if (!name.endsWith(DOT + DESC_FIELD) && name.contains(DOT)) {
final int sep = name.lastIndexOf('.');
String fieldVal = name.substring(sep + 1);
// then replace by .DESC
// Determine DESC field include dot in case of variable name such as variableEGUName
desc_name = name.replace(DOT + fieldVal, DOT + DESC_FIELD);
}

try
{
final PV new_desc_pv = PVPool.getPV(desc_name);
Expand All @@ -175,6 +196,27 @@ private void createPVs(final String name)
}
}
}

private void updateDescription() {
if(desc_value == null) {
//update description from value or pv
VType currentValue = getValue();
if(currentValue != null) {
PV thePV = pv.get();
// DisplayProvider is an optional interface for VType values,
// not PVs, but the custum datasource as Muscade happens to implement
// DisplayProvider for enum and bool PVs, so check for that here
Display display = thePV instanceof DisplayProvider ? ((DisplayProvider) thePV).getDisplay() : null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure how this will work. A value (VType) can be a DisplayProvider. A PV will never be a DisplayProvider.

The following check of currentValue instanceof DisplayProvider makes sense, but thePV instanceof DisplayProvider should be removed

Copy link
Contributor Author

@katysaintin katysaintin Jan 12, 2025

Choose a reason for hiding this comment

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

Please , it is the only way I found to manage EnumVType for Boolean value for my own datasource Muscade. VEnum and VBoolean Type are not implemented DisplayProvider , and the description is only defined for VNumber .. while a DESC field is a common field for EPICS, so I don't know how to consider the description for bo or bi ?
MuscadePV implements DisplayProvider , and it's simplify the code, avoiding test such as if datasource type is equals "ca" or not equals "sim" "loc" ...
The other point is, that create a DESC PV is complexify a lot , the code of the custom source.
Have you got a suggestion for me to solve my problem. Furthermore this test is not impacting the functionnement.

Thank you for the suggestion, else could I approve my pull request ?
Thank you for your understanding.
Katy

Copy link
Collaborator

Choose a reason for hiding this comment

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

@shroffk: Looks like the description was added to the wrong part of the VType and/or EPICS normative types. DisplayProvider started out with units and the min/max values for display, control, alarms. Those only apply to numeric data, not strings, enumerated, booleans. Then the description was added because PVAccess happens to provide it, but you're correct, in principle there's a description for bool/enum/string, not just numerics.

Alright, the PV Table cannot fix what's broken in channel access/pv access/normative types/vtypes. Still, maybe add a comment to the thePV instanceof DisplayProvider:

// DisplayProvider is an optional interface for VType values,
// not PVs, but the Muscade datasource happens to implement
// DisplayProvider for enum and bool PVs, so check for that here 

Copy link
Member

Choose a reason for hiding this comment

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

I agree that this seems like something not handled well by VType... primarily cause the description is available differently in ca vs pva

What I think would be the best way to address this would be to remove the description from the display.

// DescriptionProvider interface
public interface DescriptionProvider {
    // Method to get a description
    String getDesc();
}

// DisplayProvider interface extending DescriptionProvider
public interface DisplayProvider extends DescriptionProvider {
    // Method to get a display
    Display getDisplay();
}

So, enums, bool, strings can implement just the DescriptionProvider and not have to provide a display which makes no sense for these types.

I am afraid how much impact such a change would have on our codeabse

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, a DescriptionProvider would be a better approach.
Agree that this would be a painful change that also extends into the normative types/PVAccess, because it has the same idea for "display" information.
Let's defer that by about 5 years.

Copy link
Contributor Author

@katysaintin katysaintin Jan 14, 2025

Choose a reason for hiding this comment

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

Hello @kasemir and @shroffk ,

I'm totally agree with you about DescriptionProvider interface addition in core-pva .
I think it is the proper way to solve my issue.
And I also agree with that the impact on epics vtype clients could be problematic.

Thank you for your help, I'm OK with the comment , for clarifying the situation.
@shroffk I will make a pull request on core-epics vtype submodule.
Have a good day and thank you again.

Katy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shroffk: Looks like the description was added to the wrong part of the VType and/or EPICS normative types. DisplayProvider started out with units and the min/max values for display, control, alarms. Those only apply to numeric data, not strings, enumerated, booleans. Then the description was added because PVAccess happens to provide it, but you're correct, in principle there's a description for bool/enum/string, not just numerics.

Alright, the PV Table cannot fix what's broken in channel access/pv access/normative types/vtypes. Still, maybe add a comment to the thePV instanceof DisplayProvider:

// DisplayProvider is an optional interface for VType values,
// not PVs, but the Muscade datasource happens to implement
// DisplayProvider for enum and bool PVs, so check for that here 

@shroffk & @kasemir
I add the comment you ve ask in previous comment.
Is ok for you , in waiting for VType's modifiations ?

Thank you again for your help :)

display = display == null && currentValue instanceof DisplayProvider ? ((DisplayProvider) currentValue).getDisplay(): display;
if (display != null) {
String description = display.getDescription();
desc_value = description != null ? description : null;
desc_name = desc_value != null ? "Description of " + name + " PV" : "no description";
desc_flow = value_flow;
}
}
}
}

/** @return <code>true</code> if item is selected to be restored */
public boolean isSelected()
Expand Down Expand Up @@ -240,9 +282,13 @@ public VType getValue()
}

/** @return Description */
public String getDescription()
{
return desc_value;
public String getDescription() {
return desc_value == null ? "" : desc_value;
}

/** @return description pv name **/
public String getDescriptionName() {
return desc_name;
}

/** @return Enum options for current value, <code>null</code> if not enumerated */
Expand Down Expand Up @@ -285,14 +331,17 @@ public void setValue(String new_value)
throw new Exception("Not connected");

final VType pv_type = the_pv.read();
if (pv_type instanceof VNumber)
{
if (Settings.show_units)
{ // Strip units so that only the number gets written
final String units = ((VNumber)pv_type).getDisplay().getUnit();
if (units.length() > 0 && new_value.endsWith(units))
Display display = the_pv instanceof DisplayProvider ? ((DisplayProvider) the_pv).getDisplay() : null;
display = display == null && pv_type instanceof DisplayProvider ? ((DisplayProvider) pv_type).getDisplay():null;

if (display != null && Settings.show_units) {
// Strip units so that only the number gets written
final String units = display.getUnit();
if (units.length() > 0 && new_value.endsWith(units))
new_value = new_value.substring(0, new_value.length() - units.length()).trim();
}
if (pv_type instanceof VNumber)
{
the_pv.write(Double.parseDouble(new_value));
}
else if (pv_type instanceof VEnum)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ public class Messages
{
/** Externalized strings */
public static String Alarm,
AddPVList,
Description,
CheckAll,
CheckAll_TT,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,9 @@
import javafx.scene.control.TableRow;
import javafx.scene.control.TableView;
import javafx.scene.control.TableView.TableViewSelectionModel;
import javafx.scene.control.TextArea;
import javafx.scene.control.TextField;
import javafx.scene.control.TextInputControl;
import javafx.scene.control.ToolBar;
import javafx.scene.control.Tooltip;
import javafx.scene.control.cell.TextFieldTableCell;
Expand All @@ -83,8 +85,6 @@
import javafx.scene.paint.Color;
import javafx.util.converter.DefaultStringConverter;

import static org.phoebus.ui.application.PhoebusApplication.logger;


/** PV Table and its toolbar
* @author Kay Kasemir
Expand All @@ -95,6 +95,7 @@ public class PVTable extends VBox
private static final String comment_style = "-fx-text-fill: blue;";
private static final String new_item_style = "-fx-text-fill: gray;";
private static final String changed_style = "-fx-background-color: -fx-table-cell-border-color, cyan;-fx-background-insets: 0, 0 0 1 0;";
private static final String SPLIT_PV = "[ \\t\\n\\r,]+";

/** When sorting, keep the 'NEW_ITEM' row at the bottom **/
private static final Comparator<TableItemProxy> SORT_NEW_ITEM_LAST = (a, b) ->
Expand Down Expand Up @@ -164,12 +165,31 @@ protected void updateItem(final Boolean selected, final boolean empty)
}
}
}

private static class DescriptionTableCell extends TableCell<TableItemProxy, String>
{
@Override
protected void updateItem(final String item, final boolean empty)
{
super.updateItem(item, empty);
setText(item);
final int row = getIndex();
final List<TableItemProxy> items = getTableView().getItems();
if (! empty && row >= 0 && row < items.size()) {
final TableItemProxy itemCell = items.get(row);
if(itemCell != null && itemCell.getItem() != null) {
setTooltip(new Tooltip(itemCell.getItem().getDescriptionName()));
}
}
}
}

/** Table cell for 'name' column, colors comments */
private static class PVNameTableCell extends TextFieldTableCell<TableItemProxy, String>
{
private TextField textField;

private TextInputControl textField;
private static ContextMenu contextMenu;

public PVNameTableCell()
{
super(new DefaultStringConverter());
Expand All @@ -179,11 +199,26 @@ public PVNameTableCell()
public void startEdit()
{
super.startEdit();
textField = new TextField();
textField.setOnAction(event -> commitEdit(textField.getText()));
final int index = getIndex();
boolean newPv = index == getTableView().getItems().size() - 1;
if(newPv) {
textField = new TextArea();
textField.setMaxHeight(100);
if(contextMenu == null) {
MenuItem addPVMenu = new MenuItem(Messages.AddPVList);
addPVMenu.setOnAction(event -> commitEdit(textField.getText()));
contextMenu = new ContextMenu(addPVMenu);
}
textField.setContextMenu(contextMenu);
}
else {
textField = new TextField();
((TextField)textField).setOnAction(event -> commitEdit(textField.getText()));
}
PVAutocompleteMenu.INSTANCE.attachField(textField);
showCurrentValue();
}


private void showCurrentValue()
{
Expand Down Expand Up @@ -759,8 +794,14 @@ private void createTableColumns()
final TableItemProxy proxy = event.getRowValue();
if (proxy == TableItemProxy.NEW_ITEM)
{
if (!new_name.isEmpty())
model.addItem(new_name);
if (!new_name.isEmpty()) {
//Can be a list of pv
final String[] pvs = new_name.split(SPLIT_PV);
//Add a list
for(String pv : pvs) {
model.addItem(pv);
}
}
// else: No name entered, do nothing
}
else
Expand All @@ -784,6 +825,7 @@ private void createTableColumns()
{
col = new TableColumn<>(Messages.Description);
col.setCellValueFactory(cell -> cell.getValue().desc_value);
col.setCellFactory(column -> new DescriptionTableCell());
table.getColumns().add(col);
}

Expand Down Expand Up @@ -959,7 +1001,7 @@ else if (db.hasString())

private void addPVsFromString(final PVTableItem existing, final String pv_text)
{
final String[] pvs = pv_text.split("[ \\t\\n\\r,]+");
final String[] pvs = pv_text.split(SPLIT_PV);
for (String pv : pvs)
if (! pv.isEmpty())
model.addItemAbove(existing, pv);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
Alarm=Alarm
AddPVList=Add a list of pv
Description=Description
CheckAll=Check All
CheckAll_TT=Check all PVs in table
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
Alarm=Alarme
AddPVList=Ajout d une liste de pv
Description=Description
CheckAll=Tout sélectionner
CheckAll_TT=Sélectionner tous les PVs dans le tableau
Expand Down
5 changes: 5 additions & 0 deletions jitpack.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
before_install:
# update maven version 3.9.9
- sdk install maven 3.9.9
install:
- mvn install -DskipTests
Loading