PR-786 Code review feedback and fixes

This commit is contained in:
Garret Fick 2019-11-20 10:31:33 -05:00
parent 303d641b86
commit 8759699182
No known key found for this signature in database
GPG Key ID: A1BBEF9D2AB249C6
8 changed files with 60 additions and 53 deletions

View File

@ -140,7 +140,7 @@ void bootstrap() {
// Lock memory to ensure no swapping is done.
spdlog::info("Locking main thread memory");
if (mlockall(MCL_FUTURE|MCL_CURRENT)) {
spdlog::warn("WARNING: Failed to lock memory");
spdlog::warn("Failed to lock memory");
}
#endif

View File

@ -44,13 +44,13 @@ const GlueVariable* GlueVariablesBinding::find(IecLocationDirection dir,
return nullptr;
}
const GlueVariable* GlueVariablesBinding::find(const std::string& loc) const {
if (loc.length() < 4 || loc[0] != '%') {
const GlueVariable* GlueVariablesBinding::find(const string& location) const {
if (location.length() < 4 || location[0] != '%') {
return nullptr;
}
IecLocationDirection direction;
switch (loc[1]) {
switch (location[1]) {
case 'I':
direction = IECLDT_IN;
break;
@ -65,7 +65,7 @@ const GlueVariable* GlueVariablesBinding::find(const std::string& loc) const {
}
IecLocationSize size;
switch (loc[2]) {
switch (location[2]) {
case 'X':
size = IECLST_BIT;
break;
@ -86,11 +86,11 @@ const GlueVariable* GlueVariablesBinding::find(const std::string& loc) const {
}
char* end_msi;
uint16_t msi = strtol(loc.c_str() + 3, &end_msi, 10);
uint16_t msi = strtol(location.c_str() + 3, &end_msi, 10);
// Do we have more characters left in the string to read for lsi?
size_t start_lsi = end_msi + 1 - loc.c_str();
if (start_lsi >= loc.length()) {
size_t start_lsi = end_msi + 1 - location.c_str();
if (start_lsi >= location.length()) {
find(direction, size, msi, 0);
}

View File

@ -67,11 +67,12 @@ static char* istream_fgets(char* str, int num, void* stream) {
typedef std::unique_ptr<std::istream, std::function<void(std::istream*)>> config_stream;
/// Open the standard configuration file as an closable stream.
/// @return A stream for the configuration file.
inline config_stream open_config() {
return config_stream(
new std::ifstream("../etc/config.ini"),
[](std::istream* s)
{
[] (std::istream* s) {
reinterpret_cast<std::ifstream*>(s)->close();
delete s;
}

View File

@ -5,7 +5,7 @@
// You may obtain a copy of the License at
//
// http ://www.apache.org/licenses/LICENSE-2.0
//
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
@ -26,8 +26,8 @@
* @{
*/
//Internal buffers for I/O and memory. These buffers are defined in the
//auto-generated glueVars.cpp file
// Internal buffers for I/O and memory. These buffers are defined in the
// auto-generated glueVars.cpp file
#define BUFFER_SIZE 1024
/*********************/
/* IEC Types defs */
@ -53,30 +53,30 @@ typedef uint64_t IEC_LWORD;
typedef float IEC_REAL;
typedef double IEC_LREAL;
//Booleans
// Booleans
extern IEC_BOOL *bool_input[BUFFER_SIZE][8];
extern IEC_BOOL *bool_output[BUFFER_SIZE][8];
//Bytes
// Bytes
extern IEC_BYTE *byte_input[BUFFER_SIZE];
extern IEC_BYTE *byte_output[BUFFER_SIZE];
//Analog I/O
// Analog I/O
extern IEC_UINT *int_input[BUFFER_SIZE];
extern IEC_UINT *int_output[BUFFER_SIZE];
//Memory
// Memory
extern IEC_UINT *int_memory[BUFFER_SIZE];
extern IEC_DINT *dint_memory[BUFFER_SIZE];
extern IEC_LINT *lint_memory[BUFFER_SIZE];
//Special Functions
// Special Functions
extern IEC_LINT *special_functions[BUFFER_SIZE];
//lock for the buffer
// lock for the buffer
extern std::mutex bufferLock;
//Common task timer
// Common task timer
extern unsigned long long common_ticktime__;
struct GlueVariable;
@ -88,25 +88,25 @@ extern const char OPLCGLUE_MD5_DIGEST[];
//----------------------------------------------------------------------
//FUNCTION PROTOTYPES
// FUNCTION PROTOTYPES
//----------------------------------------------------------------------
//MatIEC Compiler
// MatIEC Compiler
extern "C" {
void config_run__(unsigned long tick);
void config_init__(void);
}
//glueVars.cpp
// glueVars.cpp
void glueVars();
void updateTime();
//hardware_layer.cpp
// hardware_layer.cpp
void initializeHardware();
void finalizeHardware();
void updateBuffersIn();
void updateBuffersOut();
//custom_layer.h
// custom_layer.h
void initCustomLayer();
void updateCustomIn();
void updateCustomOut();
@ -115,31 +115,31 @@ extern int ignored_bool_outputs[];
extern int ignored_int_inputs[];
extern int ignored_int_outputs[];
//main.cpp
// main.cpp
void sleep_until(struct timespec *ts, int delay);
bool pinNotPresent(int *ignored_vector, int vector_size, int pinNumber);
extern uint8_t run_openplc;
void handleSpecialFunctions();
//server.cpp
// server.cpp
typedef int (*process_message_fn)(unsigned char *buffer, int buffer_size, void* user_data);
void startServer(uint16_t port, volatile bool& run_server, process_message_fn process_message, void* user_data);
int getSO_ERROR(int fd);
void closeSocket(int fd);
bool SetSocketBlockingEnabled(int fd, bool blocking);
//interactive_server.cpp
void initialize_logging(int argc,char **argv);
// interactive_server.cpp
void initialize_logging(int argc, char **argv);
extern bool run_enip;
extern time_t start_time;
//enip.cpp
// enip.cpp
int processEnipMessage(unsigned char *buffer, int buffer_size, void* user_data);
//pccc.cpp ADDED Ulmer
// pccc.cpp ADDED Ulmer
uint16_t processPCCCMessage(unsigned char *buffer, int buffer_size);
//modbus_master.cpp
// modbus_master.cpp
void initializeMB();
void *querySlaveDevices(void *arg);
void updateBuffersIn_MB();

View File

@ -153,15 +153,12 @@ int main(int argc, char **argv) {
// MAIN LOOP
//======================================================
while (run_openplc) {
// Read input image - this method tries to get the lock
// so don't put it in the lock context.
updateBuffersIn();
{
std::lock_guard<std::mutex> guard(bufferLock);
// Make sure the buffer pointers are correct and
// attached to the user variables
glueVars();
// Read input image
updateBuffersIn();
updateCustomIn();
// Update input image table with data from slave devices
updateBuffersIn_MB();
@ -171,11 +168,12 @@ int main(int argc, char **argv) {
updateCustomOut();
// Update slave devices with data from the output image table
updateBuffersOut_MB();
// Write output image
updateBuffersOut();
}
// Write output image - this method tries to get the lock
// so don't put it in the lock context.
updateBuffersOut();
updateTime();
sleep_until(&timer_start, common_ticktime__);

View File

@ -27,6 +27,7 @@
#include <chrono>
#include <istream>
#include <fstream>
#include <memory>
#include <mutex>
#include <thread>
#include <type_traits>
@ -159,9 +160,9 @@ size_t pstorage_copy_glue(const GlueVariablesBinding& bindings, char* buffer) {
/// This is populated with values from the config file.
struct PstorageConfig {
PstorageConfig() :
poll_interval(std::chrono::seconds(10))
poll_interval(chrono::seconds(10))
{}
std::chrono::seconds poll_interval;
chrono::seconds poll_interval;
};
int pstorage_cfg_handler(void* user_data, const char* section,
@ -175,7 +176,7 @@ int pstorage_cfg_handler(void* user_data, const char* section,
if (strcmp(name, "poll_interval") == 0) {
// We do not allow a poll period of less than 1 second as that
// might cause lock contention problems.
config->poll_interval = std::chrono::seconds(max(1, atoi(value)));
config->poll_interval = chrono::seconds(max(1, atoi(value)));
} else if (strcmp(name, "enabled") == 0) {
// Nothing to do here - we already know this is enabled
} else {
@ -190,7 +191,7 @@ int8_t pstorage_run(oplc::config_stream& cfg_stream,
const char* cfg_overrides,
const GlueVariablesBinding& bindings,
volatile bool& run,
function<std::ostream*(void)> stream_fn) {
function<ostream*(void)> stream_fn) {
PstorageConfig config;
ini_parse_stream(oplc::istream_fgets, cfg_stream.get(),
pstorage_cfg_handler, &config);
@ -200,9 +201,10 @@ int8_t pstorage_run(oplc::config_stream& cfg_stream,
cfg_stream.reset(nullptr);
if (strlen(cfg_overrides) > 0) {
config.poll_interval = std::chrono::seconds(max(1, atoi(cfg_overrides)));
config.poll_interval = chrono::seconds(max(1, atoi(cfg_overrides)));
}
const char endianness_header[2] = { IS_BIG_ENDIAN, '\n'};
// This isn't ideal because we really only need enough space for

View File

@ -59,7 +59,11 @@ ServiceDefinition::ServiceDefinition(const char* name,
running(false),
thread(0),
config_buffer()
{}
{
GlueVariablesBinding bindings(&bufferLock, OPLCGLUE_GLUE_SIZE,
oplc_glue_vars, OPLCGLUE_MD5_DIGEST);
this->finalize_fn(bindings);
}
void ServiceDefinition::initialize() {
GlueVariablesBinding bindings(&bufferLock, OPLCGLUE_GLUE_SIZE,

View File

@ -22,6 +22,8 @@
#include "../modbusslave/slave.h"
#include "../dnp3s/dnp3.h"
using namespace std;
ServiceInitFunction pstorage_init_fn(pstorage_service_init);
ServiceStartFunction pstorage_start_service_fn(pstorage_service_run);
ServiceStartFunction dnp3s_start_service_fn(dnp3s_service_run);
@ -38,27 +40,27 @@ ServiceDefinition* services[] = {
};
ServiceDefinition* services_find(const char* name) {
ServiceDefinition** item = std::find_if(std::begin(services), std::end(services), [name] (ServiceDefinition* def) {
ServiceDefinition** item = find_if(begin(services), end(services), [name] (ServiceDefinition* def) {
return strcmp(def->id(), name) == 0;
});
return (item != std::end(services)) ? *item : nullptr;
return (item != end(services)) ? *item : nullptr;
}
void services_stop() {
std::for_each(std::begin(services), std::end(services), [] (ServiceDefinition* def) {
for_each(begin(services), end(services), [] (ServiceDefinition* def) {
def->stop();
});
}
void services_init() {
std::for_each(std::begin(services), std::end(services), [] (ServiceDefinition* def) {
for_each(begin(services), end(services), [] (ServiceDefinition* def) {
def->initialize();
});
}
void services_finalize() {
std::for_each(std::begin(services), std::end(services), [] (ServiceDefinition* def) {
for_each(begin(services), end(services), [] (ServiceDefinition* def) {
def->finalize();
});
}