Observe and manipulate boolean and string variables #257
Conversation
Combined override_manipulator::reset_xxx into one method.
include/cse.h
Outdated
| size_t nv, | ||
| bool values[]); | ||
|
|
||
| /// A struct used for retrieving string variable values with `cse_observer_slave_get_string()` |
There was a problem hiding this comment.
While I like the simplicity of this solution, I think it's too restrictive. It's very hard to determine what is a "good" size for the cse_string_value::value array. Make it too small, and you'll just get bogus data from some simulators; make it too big, and you're wasting memory. And the choice of SLAVE_NAME_MAX_SIZE seems somewhat arbitrary/unjustified.
Instead, I suggest we take our cue from FMI, in particular its fmi2GetString() function:
Let the user supply an array of const char* elements to cse_observer_slave_get_string(), which we fill with pointers to null-terminated strings. The memory for the string contents (to which the aforementioned pointers refer) is managed internally by CSE-core, typically in an internal vector of std::strings, and have a well-defined, documented lifetime. Depending on the implementation, in particular, where the strings are stored and when they get filled/overwritten, the returned pointers could be valid until:
- the next
cse_observer_slave_get_string()call for the givencse_observer - the next call to any CSE function
- the next time step
- ...and lots of other alternatives
There was a problem hiding this comment.
Thanks for the suggestion! I agree, will implement.
src/c/cse.cpp
Outdated
| std::vector<std::string> s(nv); | ||
| obs->get_string(slave, gsl::make_span(variables, nv), gsl::span<std::string>(s)); | ||
| for (size_t i = 0; i < nv; i++) { | ||
| strncpy(values[i].value, s[i].c_str(), SLAVE_NAME_MAX_SIZE); |
There was a problem hiding this comment.
strncpy() is a treacherous function. If s[i].size() >= SLAVE_NAME_MAX_SIZE, the contents of values[i].value won't be null terminated. And a C string which is not null terminated is a buffer overrun just waiting to happen.
Therefore, you should always manually set the last element in the buffer to null when using it like this. That is, the next line should be
values[i].value[SLAVE_NAME_MAX_SIZE-1] = '\0';(You may also want to use sizeof values[i].value instead of SLAVE_NAME_MAX_SIZE to be more robust against a future change of the array size in the header.)
Of course, none of this applies if you follow my suggestion and change the way the function works entirely, but it's still worth noting for future cases.
There was a problem hiding this comment.
Nice, wasn't aware you could do that. Will keep it in mind!
…al memory management for string values.
# Conflicts: # src/cpp/cse/observer/slave_value_provider.hpp
This PR fixes #253, observing boolean and string variables with
cse::last_value_observer.The C api has been updated with getters for boolean and string variables values. The solution for the latter won't win any beauty contests, please let me know if it can/should be done in a different way.
Additionally, methods for overriding and resetting boolean and string variables have been implemented in the C api, along with some minor changes to
cse::override_manipulator. Most of the changes in cse.h are from moving code around, apologies to all.For testing, you can download a distribution from the companion cse-server-go build on Jenkins. I recommend testing with
identity.fmuas it has all the variable types we need.