From b8537be219e400e1fc33a58f78223de64173c2e0 Mon Sep 17 00:00:00 2001 From: Peter Johnson Date: Thu, 19 Jan 2017 12:30:07 -0700 Subject: [PATCH] CameraServer: Remove NT-driven settings. (#467) Unfortunately, due to the way NT synchronization is currently performed, this has unexpected and undesirable behavior: when a dashboard (or any other NT client) is left running between code restarts, when it reconnects, any code settings will be overwritten by the NT synchronization process. As fixing this will require a fairly major NT change (and likely a user-visible one), it's not desirable to do at this point in the year. Instead, disable NT driven settings entirely (e.g. make the NT interface publish only). To emphasize the read-only nature of the NT values, attempts to change the NT values will be immediately overridden by CameraServer. To better inform users about the actual property names (e.g. for use in their code), the "raw_" settings no longer have "raw_" removed from their names (they are still placed in the "RawProperty" subtable). This change also contains a couple of Java fixes: * Make getInstance() thread-safe * Properly synchronize access to m_tables between multiple threads. * Use Hashtable instead of HashMap. --- wpilibc/athena/src/CameraServer.cpp | 36 ++++----- .../edu/wpi/first/wpilibj/CameraServer.java | 74 +++++++++---------- 2 files changed, 49 insertions(+), 61 deletions(-) diff --git a/wpilibc/athena/src/CameraServer.cpp b/wpilibc/athena/src/CameraServer.cpp index 7ab586007c..6abe89eafc 100644 --- a/wpilibc/athena/src/CameraServer.cpp +++ b/wpilibc/athena/src/CameraServer.cpp @@ -171,7 +171,7 @@ static std::string PixelFormatToString(int pixelFormat) { return "Unknown"; } } - +#if 0 static cs::VideoMode::PixelFormat PixelFormatFromString(llvm::StringRef str) { if (str == "MJPEG" || str == "mjpeg" || str == "JPEG" || str == "jpeg") return cs::VideoMode::PixelFormat::kMJPEG; @@ -228,7 +228,7 @@ static cs::VideoMode VideoModeFromString(llvm::StringRef modeStr) { return mode; } - +#endif static std::string VideoModeToString(const cs::VideoMode& mode) { std::string rv; llvm::raw_string_ostream oss{rv}; @@ -261,9 +261,9 @@ static void PutSourcePropertyValue(ITable* table, const cs::VideoEvent& event, llvm::SmallString<64> infoName; if (llvm::StringRef{event.name}.startswith("raw_")) { name = "RawProperty/"; - name += llvm::StringRef{event.name}.substr(4); + name += event.name; infoName = "RawPropertyInfo/"; - infoName += llvm::StringRef{event.name}.substr(4); + infoName += event.name; } else { name = "Property/"; name += event.name; @@ -427,6 +427,9 @@ CameraServer::CameraServer() 0x4fff, true}; // Listener for NetworkTable events + // We don't currently support changing settings via NT due to + // synchronization issues, so just update to current setting if someone + // else tries to change it. llvm::SmallString<64> buf; m_tableListener = nt::AddEntryListener( Concatenate(kPublishName, "/", buf), @@ -446,24 +449,16 @@ CameraServer::CameraServer() relativeKey = relativeKey.substr(subKeyIndex + 1); // handle standard names - llvm::SmallString<64> propNameBuf; llvm::StringRef propName; if (relativeKey == "mode") { - if (!value->IsString()) return; - auto mode = VideoModeFromString(value->GetString()); - if (mode.pixelFormat == cs::VideoMode::PixelFormat::kUnknown || - !sourceIt->second.SetVideoMode(mode)) { - // reset to current mode - nt::SetEntryValue(key, nt::Value::MakeString(VideoModeToString( - sourceIt->second.GetVideoMode()))); - } + // reset to current mode + nt::SetEntryValue(key, nt::Value::MakeString(VideoModeToString( + sourceIt->second.GetVideoMode()))); return; } else if (relativeKey.startswith("Property/")) { propName = relativeKey.substr(9); } else if (relativeKey.startswith("RawProperty/")) { - propNameBuf = "raw_"; - propNameBuf += relativeKey.substr(12); - propName = propNameBuf.str(); + propName = relativeKey.substr(12); } else { return; // ignore } @@ -474,17 +469,14 @@ CameraServer::CameraServer() case cs::VideoProperty::kNone: return; case cs::VideoProperty::kBoolean: - if (!value->IsBoolean()) return; - property.Set(value->GetBoolean() ? 1 : 0); + nt::SetEntryValue(key, nt::Value::MakeBoolean(property.Get() != 0)); return; case cs::VideoProperty::kInteger: case cs::VideoProperty::kEnum: - if (!value->IsDouble()) return; - property.Set(static_cast(value->GetDouble())); + nt::SetEntryValue(key, nt::Value::MakeDouble(property.Get())); return; case cs::VideoProperty::kString: - if (!value->IsString()) return; - property.SetString(value->GetString()); + nt::SetEntryValue(key, nt::Value::MakeString(property.GetString())); return; default: return; diff --git a/wpilibj/src/athena/java/edu/wpi/first/wpilibj/CameraServer.java b/wpilibj/src/athena/java/edu/wpi/first/wpilibj/CameraServer.java index c7ee248624..f0ec0d2b99 100644 --- a/wpilibj/src/athena/java/edu/wpi/first/wpilibj/CameraServer.java +++ b/wpilibj/src/athena/java/edu/wpi/first/wpilibj/CameraServer.java @@ -25,7 +25,7 @@ import edu.wpi.first.wpilibj.networktables.NetworkTable; import edu.wpi.first.wpilibj.networktables.NetworkTablesJNI; import edu.wpi.first.wpilibj.tables.ITable; import java.util.ArrayList; -import java.util.HashMap; +import java.util.Hashtable; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -49,7 +49,7 @@ public class CameraServer { /** * Get the CameraServer instance. */ - public static CameraServer getInstance() { + public static synchronized CameraServer getInstance() { if (server == null) { server = new CameraServer(); } @@ -57,12 +57,12 @@ public class CameraServer { } private String m_primarySourceName; - private HashMap m_sources; - private HashMap m_sinks; - private HashMap m_tables; // indexed by source handle - private ITable m_publishTable; - private VideoListener m_videoListener; //NOPMD - private int m_tableListener; //NOPMD + private final Hashtable m_sources; + private final Hashtable m_sinks; + private final Hashtable m_tables; // indexed by source handle + private final ITable m_publishTable; + private final VideoListener m_videoListener; //NOPMD + private final int m_tableListener; //NOPMD private int m_nextPort; private String[] m_addresses; @@ -93,11 +93,6 @@ public class CameraServer { return "mjpg:http://" + address + ":" + port + "/?action=stream"; } - @SuppressWarnings("JavadocMethod") - private synchronized ITable getSourceTable(int source) { - return m_tables.get(source); - } - @SuppressWarnings({"JavadocMethod", "PMD.AvoidUsingHardCodedIP"}) private synchronized String[] getSinkStreamValues(int sink) { // Ignore all but MjpegServer @@ -290,8 +285,8 @@ public class CameraServer { String name; String infoName; if (event.name.startsWith("raw_")) { - name = "RawProperty/" + event.name.substring(4); - infoName = "RawPropertyInfo/" + event.name.substring(4); + name = "RawProperty/" + event.name; + infoName = "RawPropertyInfo/" + event.name; } else { name = "Property/" + event.name; infoName = "PropertyInfo/" + event.name; @@ -335,9 +330,9 @@ public class CameraServer { @SuppressWarnings({"JavadocMethod", "PMD.UnusedLocalVariable"}) private CameraServer() { - m_sources = new HashMap(); - m_sinks = new HashMap(); - m_tables = new HashMap(); + m_sources = new Hashtable(); + m_sinks = new Hashtable(); + m_tables = new Hashtable(); m_publishTable = NetworkTable.getTable(kPublishName); m_nextPort = kBasePort; m_addresses = new String[0]; @@ -359,9 +354,7 @@ public class CameraServer { case kSourceCreated: { // Create subtable for the camera ITable table = m_publishTable.getSubTable(event.name); - synchronized (this) { - m_tables.put(event.sourceHandle, table); - } + m_tables.put(event.sourceHandle, table); table.putString("source", makeSourceValue(event.sourceHandle)); table.putString("description", CameraServerJNI.getSourceDescription(event.sourceHandle)); @@ -373,7 +366,7 @@ public class CameraServer { break; } case kSourceDestroyed: { - ITable table = getSourceTable(event.sourceHandle); + ITable table = m_tables.get(event.sourceHandle); if (table != null) { table.putString("source", ""); table.putStringArray("streams", new String[0]); @@ -382,7 +375,7 @@ public class CameraServer { break; } case kSourceConnected: { - ITable table = getSourceTable(event.sourceHandle); + ITable table = m_tables.get(event.sourceHandle); if (table != null) { // update the description too (as it may have changed) table.putString("description", @@ -392,42 +385,42 @@ public class CameraServer { break; } case kSourceDisconnected: { - ITable table = getSourceTable(event.sourceHandle); + ITable table = m_tables.get(event.sourceHandle); if (table != null) { table.putBoolean("connected", false); } break; } case kSourceVideoModesUpdated: { - ITable table = getSourceTable(event.sourceHandle); + ITable table = m_tables.get(event.sourceHandle); if (table != null) { table.putStringArray("modes", getSourceModeValues(event.sourceHandle)); } break; } case kSourceVideoModeChanged: { - ITable table = getSourceTable(event.sourceHandle); + ITable table = m_tables.get(event.sourceHandle); if (table != null) { table.putString("mode", videoModeToString(event.mode)); } break; } case kSourcePropertyCreated: { - ITable table = getSourceTable(event.sourceHandle); + ITable table = m_tables.get(event.sourceHandle); if (table != null) { putSourcePropertyValue(table, event, true); } break; } case kSourcePropertyValueUpdated: { - ITable table = getSourceTable(event.sourceHandle); + ITable table = m_tables.get(event.sourceHandle); if (table != null) { putSourcePropertyValue(table, event, false); } break; } case kSourcePropertyChoicesUpdated: { - ITable table = getSourceTable(event.sourceHandle); + ITable table = m_tables.get(event.sourceHandle); if (table != null) { String[] choices = CameraServerJNI.getEnumPropertyChoices(event.propertyHandle); table.putStringArray("PropertyInfo/" + event.name + "/choices", choices); @@ -450,8 +443,11 @@ public class CameraServer { }, 0x4fff, true); // Listener for NetworkTable events + // We don't currently support changing settings via NT due to + // synchronization issues, so just update to current setting if someone + // else tries to change it. m_tableListener = NetworkTablesJNI.addEntryListener(kPublishName + "/", - (uid, key, value, flags) -> { + (uid, key, eventValue, flags) -> { String relativeKey = key.substring(kPublishName.length() + 1); // get source (sourceName/...) @@ -471,16 +467,13 @@ public class CameraServer { // handle standard names String propName; if (relativeKey.equals("mode")) { - VideoMode mode = videoModeFromString((String) value); - if (mode.pixelFormat == PixelFormat.kUnknown || !source.setVideoMode(mode)) { - // reset to current mode - NetworkTablesJNI.putString(key, videoModeToString(source.getVideoMode())); - } + // reset to current mode + NetworkTablesJNI.putString(key, videoModeToString(source.getVideoMode())); return; } else if (relativeKey.startsWith("Property/")) { propName = relativeKey.substring(9); } else if (relativeKey.startsWith("RawProperty/")) { - propName = "raw_" + relativeKey.substring(12); + propName = relativeKey.substring(12); } else { return; // ignore } @@ -491,14 +484,17 @@ public class CameraServer { case kNone: return; case kBoolean: - property.set(((Boolean) value).booleanValue() ? 1 : 0); + // reset to current setting + NetworkTablesJNI.putBoolean(key, property.get() != 0); return; case kInteger: case kEnum: - property.set(((Double) value).intValue()); + // reset to current setting + NetworkTablesJNI.putDouble(key, property.get()); return; case kString: - property.setString((String) value); + // reset to current setting + NetworkTablesJNI.putString(key, property.getString()); return; default: return;