Top level API for wolf
Created by: joansola
EDIT: The keys to factories are strings, and the sensor params to be passed to the factory methods are filenames, so that we can pass YAML files directly
We need to create a WOLF api for users. A minimal skeleton of the ROS node starts by configuring the problem, as follows:
Problem problem;
problem.addSensor("SENSOR TYPE 1", "SensorName1", "sen1/params/filename");
problem.addSensor("SENSOR TYPE 2", "SensorName2", "sen2/params/filename");
[... more sensors ...];
problem.addProcessor("PROCESSOR TYPE 1", "ProcName1", "SenName1", "prc1/params1/filename");
problem.addProcessor("PROCESSOR TYPE 2", "ProcName2", "SenName2", "prc2/params2/filename");
[... more processors ...];The methods addSensor() and addProcessor() act as factories. Their design is as follows.
- First, put two factories in Problem:
````
Problem::addSensor(string _type, string _sensor_name, string _params_filename)
{
SensorBase* sen_ptr = sensor_factory.createSensor( _type, _sensor_name, _params_filename);
this->addSensor(sen_ptr);
};
Problem::addProcessor(string _type, string _processor_name, string _sensor_name, string _params_filename)
{ idem... };
````- Then, write individual factory methods
createCamera(), createIMU(), ... etc ...that will be defined in each specific sensor class, or in the file of that class. - Also,
Problemneeds two methodsregisterSensorFactory()andregisterProcessorFactory(), with the appropriate API, that registers thecreateXxxx()methods of each class that can be created in the factory. EDIT: These are not required. See post below about registering the methods to the factories. - Finally, at any given project, someone has to call, before all configuration starts, all the necessary
registerXxxxxFactory(xxxx). EDIT: This is not required. Only #include the desired headers. See post below about registering.
Activity
Created by: joansola
The Factory methods are registered automatically with a static call (a strange feature that works). The following code does the trick. This code is in each
SensorDerived.hfile, just after the class declaration:// Define the factory method and register it in the SensorFactory namespace { // This is the static factory method: SensorBase* createCamera(std::string& _name) { SensorBase* sen = new SensorCamera(nullptr, nullptr, nullptr,0,0); sen->setName(_name); return sen; } // This is the static call that registers the factory method to the factory: const bool registered_camera = SensorFactory::get()->registerSensor("CAMERA", createCamera); }In conclusion, the factory methods are registered by just
#include-ing the header files of the desired sensors and processors.Then the Factory methods are simply called in
main()or in the ROS node, to add sensors and processors to the problem, with the following API:#include "sensor_type.h" #include "processor_type.h" void main(){ Problem problem(FRM_POV_3D); problem.addSensor("SENSOR TYPE", "Sensor Name", "sensor_params_filename.yaml") problem.addProcessor("PROCESSOR TYPE", "Processor Name", "Sensor Name", "processor_params_filename.yaml") }The call to
addProcessor()automatically binds the processor to the sensor by appropriately detecting the"Sensor Name"and doing the association.-
Created by: joansola
We'll have soon a
SensorFactorybased onYAMLconfiguration files. I post here some design possibilities that we need to agree:- API : please read two posts above, I have a proposition that looks good.
- file organization: we need to be organized. The
Yamlneeds an external library,yaml-cpp, which is maybe not needed if we work on ROS, as ros already implements the same thing throughrosparamsI think. therefore, we can keep the dependence onyaml-cppas optional, and this means that some methods need to be out of the mainSensorCamera[etc...] class files. - yaml file format. Please take a look at
src/examples/camera.yamlin branchfactory. You can also check outtest_yaml.cppto see how the file is parsed and the parameters extracted.
-
Created by: joansola
The API is converging to the following:
- No YAML files.
-
Instead, we will use structs derived from IntrinsicsBase, which is an empty struct defined in sensor_base.h.
-
Define your IntrinsicsDerived strucs in your sensor_xxx.h file, just before SensorXxx class,
```` struct IntrinsicsCamera : public IntrinsicsBase { ... } ````
-
In
Problem, we have theaddSensor()method. This is the API:SensorBase* addSensor(string _type, string _name, VectorXs& _extrinsics, IntrinsicsBase* _intrinsics) -
A similar API will be defined for the
ProcessorXxxclasses. -
In our main(), ROS node, or whatever the application scheme we use, we need to:
-
register the sensor creator to the factory. This is done by just
#include-ing the header files of our specific sensors, with#include "sensor_xxx.h". -
create the sensors and register them to Problem. This is done by
problem.addSensor(...). -
create the Processors, and bind them to the sensors. This is done by
problem.addProcessor(...).
Here is an example of creating two sensor cameras
void main(){ #include "sensor_factory.h" #include "sensor_camera.h" // provides SensorCamera and registers the pair <"CAMERA", SensorCamera::create> in the Sensor factory. // Note: SensorCamera::create() is already registered. //To invoke it, use the std::string "CAMERA" as in the lines below. // We create two cameras... // Fisrt camera... Eigen::VectorXs extrinsics(7); // give it some values... IntrinsicsCamera intrinsics({...}); // also fill in the derived struct (here we suggested the struct initializer with '{}')... // To create a camera, provide "TYPE", "Name", extrinsics vector, and a pointer to the intrinsics struct: SensorFactory::get()->create("CAMERA", "Front-left camera", extrinsics, &intrinsics); // Second camera... with a different name! extrinsics = ...; // give it some other values... intrinsics = ...; // also fill in the derived struct for the second camera... SensorFactory::get()->create("CAMERA", "Front-right camera", extrinsics, &intrinsics); } -
Created by: joansola
Pushed to master.
@joanvallve , @andreucm , @artivis, could you please:
- Provide some feedback if you see something odd here
- If you feel like, make use of the
SensorFactorymethods in
- Your
SensorXxxclasses <-- done for the sensors currently in wolf. - Your
test_xxxx.cppfiles <-- get inspired bysrc/examples/test_sensor_factory.cpp - Your ROS nodes?
There is a generous documentation in the SensorFactory class
Created by: artivis
@joansola this looks great ! I would only have recommended to use a scoped enum for the sensor types.
Using string might forces you later to deal with boring thing. E.g see #66
-
Created by: joansola
Done the following:
- Implemented
ProcessorFactory. - Implemented
Problem.addProcessor("PRC TYPE", "Prc name", "Sen name", prc_params_ptr)
Now we need to:
- implement each
static ProcessorXxx::create()method, taking a pointer toparams, and calling theProcessorXxxconstructor (exactly as it is done in eachSensorXxx). - register each
ProcessrXxx::createinto theProcessorFactory(exactly as it is done in eachSensorXxx).
- Implemented
-
Created by: joansola
Implemented
Problem.addProcessor("TYPE", "Processor Name", "Sensor Name", proc_params_ptr), with automatinc binding to the sensor specified in"Sensor Name", and with the following processors already registered in the Factory:ProcessorOdom2DProcessorOdom3D-
ProcessorLaser2Dwe'll take care of large merge from @joanvallve . If necessary, we can re-do the job, it's easy, and Joan, you can just take your copy if you find too many conflicts. ProcessorGPS
NOTE: I think providing "Processor Name" is completely unnecessary, and the processor is conveniently identified by its type. What do you think?
-
Created by: joansola
@artivis, could you please run test_sensor_factory from branch factory, and see what happens with the multiple intent of registering? I put a std::cout comment in the test to make this visible.
In principle, the factory method for each specific sensor (or process) is registered with a static invocation in the sensor_xxx.h file (check the last lines of code of any of these files). Then, I expect that this line be executed just once, because there is the #include guard. But, surprise, I get these messages "Cannot register sensor XXX" many times, once for each time the system tried to register a factory method after a successful registration had been already done. This tells me that, regardless of the #include guard, we try to register several times! This is very odd!
Any light over this issue?
If we do not like it like this, then we'll have to register each method explicitly in our main() or ROS node. Just remove the static invocation from the .h files, and put it in our main(). But then, we'll have more lines to code in each project, and the user will have more things to remember, and we'll have more things to document for the end user.
thks.
-
Created by: joansola
I have been working with the facotry registers, with quite a lot of trouble.
As explained before, the registry is accomplished by making a call to a line such as:
SensorFactory::get()->registerCreator( "GPS" , SensorGPS::create );and the issue of this post is: where to put this line?
The first design (explained in the posts above) implemented automatic registration. This puts the registry line at the end of the header files, and uses the #include of the header files in the application to achieve automatic registration:
// ======== file sensor_gps.h ========== #ifndef SENSOR_GPS_H #define SENSOR_GPS_H class SensorGPS { ... rest of the class declaration ... }; #include "sensor_factory.h" namespace{ SensorFactory::get()->registerCreator( "GPS" , SensorGPS::create ); } #endif // ======== file application.cpp =========== #include "sensor_gps.h" // this registers SensorGPS to the factory automatically ! main () { [... rest of the code ....] }After all my investigations, I think the method of automatic regstration is quite obscure and leads to a lot of uncontrolled situations.
For example, I was including only the required headers in my application file
test_sensor_factory.cpp, but I got a bunch of successful registers and failed attempts:Registered sensor CAMERA Registered sensor ODOM 2D Registered sensor GPS FIX Registered sensor GPS Registered sensor IMU Registered processor ODOM 3D Could not register sensor CAMERA Could not register sensor CAMERA Could not register sensor IMU Registered processor IMU Registered processor ODOM 2D Could not register processor ODOM 2Dwhich means that, even though the header file is properly guarded with the include guards, I am not really controlling the registering procedure. Probably, other .cpp files, including the same headers but being part of other executables in the library, also attempt registry. This is not good.
To solve this, it is best to control the register of all factories on a per-application basis. This means including one line of code per each library to register. I see three options:
-
Inside the main():
int main () { // Register all required Sensor and Processor creators to the factories SensorFactory::get()->registerCreator("ODOM 2D", SensorOdom2D::create); SensorFactory::get()->registerCreator("IMU", SensorIMU::create); ProcessorFactory::get()->registerCreator("ODOM 2D", ProcessorOdom2D::create); ProcessorFactory::get()->registerCreator("IMU", ProcessorIMU::create); [... rest of the code ...] } ````
-
At global scope, before the main()
namespace{ // Register all required Sensor and Processor creators to the factories // note: we need to define a variable to be able to call functions here. that's why the 'const bool' const bool registered_sen_odom_2d = SensorFactory::get()->registerCreator("ODOM 2D", SensorOdom2D::create); const bool registered_sen_imu = SensorFactory::get()->registerCreator("IMU", SensorIMU::create); const bool registered_prc_odom_2d = ProcessorFactory::get()->registerCreator("ODOM 2D", ProcessorOdom2D::create); const bool registered_prc_imu = ProcessorFactory::get()->registerCreator("IMU", ProcessorIMU::create); } int main () { [... rest of the code ...] } ````
-
At global scope, in a dedicated header file
// ========= file facrory_registers.h ======= namespace{ // Register all required Sensor and Processor creators to the factories const bool registered_sen_odom_2d = SensorFactory::get()->registerCreator("ODOM 2D", SensorOdom2D::create); const bool registered_sen_imu = SensorFactory::get()->registerCreator("IMU", SensorIMU::create); const bool registered_prc_odom_2d = ProcessorFactory::get()->registerCreator("ODOM 2D", ProcessorOdom2D::create); const bool registered_prc_imu = ProcessorFactory::get()->registerCreator("IMU", ProcessorIMU::create); }
// ======= file application.cpp ======== #include "factory_registers.h" int main () { [... rest of the code ...] } ````
possibly, we want to use a different header extension here, like ````factory_registers.hpp````, to indicate that this is not a real 'header' but rather executable code that we want to encapsulate. We could also use ````.cpp```` and include it (it is not forbidden!), but this last solution would be odd IMO.All methods work fine. Their result is identical: each method is registered once, and only the wanted methods are registered. This seems OK. The result in a real situation is like:
Registered sensor CAMERA Registered sensor ODOM 2D Registered sensor GPS FIX Registered sensor GPS Registered sensor IMU Registered processor ODOM 3D Registered processor IMU Registered processor ODOM 2D ================ Wolf Factories ================ If you look above, you see the registered methods. ================ Sensor Factory ================ Sensor: 1 | type 5 : CAMERA | name: front left camera Sensor: 2 | type 5 : CAMERA | name: front right camera Sensor: 3 | type 1 : ODOM 2D | name: main odometer Sensor: 4 | type 6 : GPS FIX | name: GPS fix Sensor: 5 | type 5 : CAMERA | name: rear camera Sensor: 6 | type 4 : IMU | name: inertial Sensor: 7 | type 1 : ODOM 2D | name: aux odometer aux odometer's pointer: 0x7ff2015129f0 --------> All pointers are accessible if needed! =============== Processor Factory =============== Processor: 1 | type : ODOM 2D | name: main odometry | bound to sensor: main odometer Processor: 3 | type : IMU | name: pre-integrated | bound to sensor: inertial Processor: 2 | type : ODOM 3D | name: sec. odometry | bound to sensor: aux odometer -
Created by: artivis
I don't like both solutions 1/ and 2/ for the reason that they rely on user. This registration should be somewhat magic and hidden. As for the solution 3 it brings back the centralization that you were trying to avoid in a first place. Still that would be my favorite option.
How about designing a sensor recorder class ? A singleton, that makes the binding between the factory and the sensor create() function to be registered ? That sounds nasty be it would ensure a unique registration, and save the decentralize aspect... What do you think ?
-
Created by: joansola
Ok i like the magic automatic aspect. We need to find a nice solution
Maybe the old solution is ok, and we just revealed a problem that is causing it to work improperly. Then, finding and solving this problem would be the best solution of all.
I have the feeling that this could be solved at cmake level, as you somehow suggested too.
-
Created by: joansola
We have been discussing in commit 9e719f9
About the automatic solution based on #include only, I agree that depending on the inner logics of #including is not the way. We should enforce the behaviour we want, not depend on magic workarounds.
I checked google for 'unnamed namespace and include guards' and variants, and could not find any clue that could provide any light on the issue.
So at least we agree that we want to find a solution that is:
- automatic
- not requiring centralized knowledge
- easy to understand
- as minimalist as possible
Created by: artivis
Perfect, but there is another issue (issues?) which is that you can't do the following :
Register<SensorCamera>::register_sensor("CAMERA");Neither :Register<SensorOdom2D>* usuless = Register<SensorOdom2D>::register_sensor("ODOM 2D");Register<SensorCamera>* usuless = Register<SensorCamera>::register_sensor("CAMERA");(Same name, of course not ...).This doesn't seems to be a big deal but still annoying to have those 'usuless' ptr hanging there...
-
Created by: joansola
@artivis OK now I understand what we are doing !
I think what we do is not good either. Basically:
We enforce that, regardles of the times we call register_sensor, the sensor only tries to get registered once. OK.
But just before, we had that, regardles of how many attemps, the sensor only got registered once. Because resiterCreator() in the original factory was already checking that the method was unregistered before actually registering.
So basically, we have the same. We just added a layer, but did not get rid of the issue. We still have the system passing several times over register_sensor, the same as it was passing several times over registerCreator().
I think I want to investigate about this #include logic. If we can rely on it, then the original method was good -- just need to remove the comments saying "Could not register XXX", or replace them by "XXX already registered: skipping"
-
Created by: joansola
We have still more problems:
With the automatic registration (whatever the solution we took), no matter what test we launch, we get aaaalll the sensors being registered. It is quite stupid! If I launch test_motion, which has nothing to do with registering things in factories, I still get all these messages of sensors being registered.
I'll post another entry about what I learned about #including, unnamed namespaces, and translation units, which is why all this is happening.
-
Created by: joansola
@artivis, what about this super stupid solution?
- make folder src/autoregister/
- add files sensor_xxx.h
- these files are like this:
#ifndef AUTOREGISTER_SENSOR_ODOM_2D_H_ #define AUTOREGISTER_SENSOR_ODOM_2D_H_ #include "../sensor_odom_2D.h" // Register in the SensorFactory #include "sensor_factory.h" namespace wolf { namespace { const bool registered_odom_2d = SensorFactory::get()->registerCreator("ODOM 2D", SensorOdom2D::create); } } // namespace wolfThen in the application, instead of
#include "sensor_xxx.h"we use
#include "autoregister/sensor_xxx.h"the solution may admit some variants (e.g. not repeating the file name literally, etc), but it's kind of cool...
-
Created by: joansola
Again, I think it boils down to the same thing...
Maybe automatic registration is after all not a desirable feature --- unless we ensure that the library has no translation units including these
autoregister/versions. Now, I think it is the existence of tests #including sensors_xxx.h and processors_xxx.h what produces the multiple registration, because the static symbols get defined once per translation unit, regardless of the include guards. That's finally the cause of the issue. And the solution is to provide a library with no translation units including these registration lines !Another solution is via CMAKE: just make whatever needs to be done so that the tests_xxx.cpp are taken into account in a different way, thus not impacting on the number of translation units of the library.
I am not very sure of what I am saying...
-
Created by: joansola
I found this in http://stackoverflow.com/questions/7679297/automatic-compile-time-factory-registration-of-class-templates-in-c:
Doing simpler things will break less and makes your intentions obvious.
int main() { RegisterConcreteTypeFoo(); RegisterConcreteTypeBar(); // do stuff... CleanupFactories(); return 0; }When those init functions are actually called (not at compile time) and they fail you won't get all the pretty easy things that makes debugging easier. Like a stack trace.
With this scenario you also assume you won't want to initialize them differently. It is overly complicated to unit test "automatically registered" anything, for instance.
Less magic = easier, cheaper maintenance.
If that weren't enough, there are technical problems too. Compilers like to strip out unused symbols from libraries. There may be a compiler specific shenanigan to get around it, I'm not sure. Hopefully it does it in a consistent way, instead of randomly for no obvious reason in the middle of a development cycle.
-
Created by: joansola
OK @artivis !! I found it, and it is really easy!
Problem: multiple and uncontrolled attempts to register a creator in the factory Cause: If the registration line is in the header file, each translation unit (e.g. cpp file in the library) including
#include "sensor_xxx.h"will generate an attempt. Solution: Put the register line of the xxx creator in the cpp file of the xxx class! In our case, in the sensor_xxx.cpp file! It works, because nobody is including this cpp file, and therefore the register exists only once!It works like a charm, and it is super simple!
OK so next Monday I want to close this issue.
-
Created by: joansola
Comment by Nicolas Mansard @nmansard :
About the rest of the API: I don't like very much that the C++ code is relying directly on string to refer to already-created objects, e.g. createProcessor("ProcType", "ProcName", "SenName") I would rather suggest: createProcessor("ProcType", "ProcName",senPtr)
I guess that you would have a "pool" of object, i.e. a map<string,ptr> that stores all the objects. Then you would have something like: createProcessor("ProcType", "ProcName", senPool["SenName"]) Maybe it is too heavy.
I liked the dynamic parameters that you mentionned earlier. Don't you think it is worth formatting them properly? e.g.: procParam = createProcessorParam("ProcType", ... ) createProcessor("ProcType", "ProcName", senPool["SenName"], procParam)
-
Created by: joansola
About using names, I got inspired by OpenCV 2.4, especially on the way they handle the visualization windows, which are identified by strings. But maybe you are right. OpenCV is also taking very strange solutions when it comes to OO and encapsulation...
The solution I have is to look at the list of sensors for a sensor matching the name string, and then binding the processor with the matching sensor.
Your solution with the pointer is easy and clear.
I do not see this API of yours with senPool["Name"] clearer than mine... but the mechanism I use is essentially the same. I have a std::list<> of sensors, and I look for the name using std::find_if. The list is very short, and only used at configuration time, so I do not bother about the linear complexity of std::find_if(). Am I missing something important?
About the dynamic parameters (a pointer to a base struct of params), yes, you get the point: now,
createProcessorParam("ProcType", ...)is again a factory method. Here, the way to create these params may depend. I have three main options:- Using YAML files in WOLF directly <-- I used yaml-cpp library which is fine. The issue is: I'd like not to repeat the work done elsewhere, especially in ROS -- see next point.
- Using YAML files via ROS's rosparams, which is very powerful
- Using manual coding directly in the main(), or similar <-- ugly solution
Solution 3 is ugly but demands nothing. Solution 2 is perfect in ROS. Solution 1 might be shipped alongside WOLF, by just providing yet another prototype for Problem::addSensor("TYPE", "Name", "params_filename.yaml"). If we do so, I'd like at least that the YAML files in WOLF follow strictly the format of those in ROS, just to be as compatible as possible.
-
Created by: joansola
Reply by Nicolas Mansard @nmansard
You are right, it does not change much. I do not like having std::string in the API, as they tend to diffuse in all the API and to have finally something very weak in term of prototyping.
find_if() is ok, no big difference with map. Maybe map is easier to use. Search cost is not an issue.
About dynamic parameters:
- Solution 3 is not a solution that is compatible with the idea of factory.
- Solution 1 or 2 are pretty equivalent to me, and more a question of style. I prefer 1, as it can be used without ROS, but also can be quickly binded with ROS to obtain something very close to 2.
In general, I think that it is nice to have something just a little bit more generic than 1, with a factory of parameters. I do not think that it would add much in term of complexity. And then, if one day you want to be able to initialize an object with something else than YAML, it would be straightforward to implement.
Created by: artivis
About @nmansard comments I do agree about the strings issue, however this solution happened to be the 'least worst' so to speak. The whole point of this factory-based API was to prevent the final from dealing with any pointers.
As for the params, I doubt that we need a 'create()' function for this too. A simple
struct(members are public by default) with a clear name would do the job, e.g. CameraParameters. Finally I would also vote for the solution 1, that is using yaml file directly in wolf.-
Created by: nmansard
About the first point of @artivis : I think that the idea of the factory is to allow future users to extend the software without modifying the core library. If the idea is to avoid pointers, a simple map (or any string-indexed list) should be enough, while having all the classes inherited from the API in the core library.
A side effect of the factory is that the whole data graph can be described with only strings in a text (XML or other) file, with strings for the types and string for the object ID. This is a excellent point that should be exploited. However, I do not think that the strings should be explicitly used inside the C++ code. Pointers are more efficient, contain a lot of type information which allows in turn the compiler to check the syntax of the program.
About the second point of @artivis : a static struct is not generic enough. It might be sufficient in an initial phase, but cannot be extended. And then, it somehow "breaks" the spirit of the factory.
I think that a good way to think a factory is to always imagine how you will describe new objects or situations through an XML file, while adding new plugins (maybe you can also think about how to write a "main() function by the way, but it is generally more straightforward and less informative about the factory structure). Imagine you now want to add a magnetometer, which has completely different parameters than a camera or an IMU (it is just an example, I do not know much about this sensor): how will you describe the new sensor in the XML file. You need a new parameter object, which in turn must be added in the new plugin ...
This is not a big deal, as the factory for sensors and processors can be simply augmented to add the parameter classes in the same factory structure, or by simply copying the main factory to obtain a parameter factory.
As proposed initially by @joansola, another solution is to consider that any object might be initialize by a specific standard type (for example a string representing a file path), which then enters in the API.
Created by: artivis
@nmansard Well you are right in that the factory design aims at easing future extensions of wolf, but also avoiding the final user to play with pointers. I am not talking about a developer that would join in the future but an actual user that wants things to be simple (e.g. createSensor("my sensor", sensorType) notice that this user don't even care about intrinsics params...). Later on the user may not even call
addSensor(...)himself but instead simply fill-up a yaml file. To be convince about this fact you can check the 'issues' sections of both 'LSD-SLAM' and 'ORB-SLAM' on github.As for the params structure a meant something obviously flexible using a 'decorator pattern' or such. You can then use the base class to move the structure around. If you actually add a magnetometer you then have to create it corresponding
SensorMagnetometerclass which will access at some point theParamMagnetometermembers. It is somewhat defined then... I may have missed the point however.-
Created by: joansola
I see these options:
- In one go:
-
problem.createSensor("CAMERA", "Name", "yaml file")- we call the Factory with "CAMERA" -> get the static SensorCamera::create()
- we call SensorCamera::create("Name", "yaml file")
- we parse the yaml file --> create a struct ParamsCamera : public ParamsBase
- we call the camera constructor: SensorCamera("Name", & ParamsCamera)
PRO: one line of code as external API CON: the YAML file enters too much and everything is YAML-dependent.
- In two goes:
-
ParamsBase* params_ptr = createParams("CAMERA", "yaml file")--> create a struct ParamsCamera : public ParamsBase, return a pointer to base. -
problem.createSensor("CAMERA", "Name", params_ptr)- we call the Factory with "CAMERA" -> get SensorCamera::create()
- we call SensorCamera::create("Name", params_ptr)
- we cast the params pointer to the camera type --> camera_params_ptr
- we call the camera constructor: SensorCamera("Name", *camera_params_ptr)
PRO1: the YAML file gets decoded quickly. After that, all is only WOLF-dependent. PRO2: we can change the way params are entered by user, from wolf-yaml, to ROS-rosparams, to Foo-xml, with little impact on the rest --> just change createParams(). CON1: two lines of code as external API, CON2: a pointer appears at user level, but... PRO3: We can also overload problem.createSensor() with
problem.createSensor("CAMERA", "Name", "yaml file") { ParamsBase* params_ptr = createParams("CAMERA", "yaml file"); this->createSensor("CAMERA", "Name", params_ptr); }and achieve a single-line API at Problem level, yet conserving the original API so that all other PROs are still valid.
As per the current state of developments, we are in solution 2.
-
Created by: joansola
@artivis Regarding the decorator and the link you provided,
I see that, if we implement dynamic decorators, we are essentially doing regular dynamic polymorphism. This is what is currently designed:
struct ParamsCamera : public ParamsBase { bla bla bla }<--- you can check sensor_camera.hAs for static decorations, this boils down to CRTP in my opinion. In Wolf, we are staying away from CRTP, just for the sake of keeping away from it: dynamic polymorphism is OK and already understood by the members of the developing team -- we already discussed about this.
So my conclusion is that the design in my previous post is essentially following what you suggest in your post. Again, am I missing some important point?
As per the general API, yes, we are doing an effort of easing life to the final user. Strings and yaml files both go in this direction. Still, I think that adding a pointer to sensor would not be such a pain, but this is effectively avoidable with the methods exposed:
- overload problem.createSensor(), as in PRO3 in my previous post
- make a pool of sensors and use sensorPool("Name") to return a pointer, as @nmansard suggested, and place this little code straight in the main call,
problem.createProcessor("FEATURE TRACKER", "Name", senPool("Camera Name"), createParams("tracker_params.yaml"))At this point, I am very satisfied with the solution we have, I think it is minimalist enough from the user standpoint.
-
Created by: joansola
@nmansard and where would you put the sensorPool() method?
- If it is a static method, then the API is small, but the method is flying around.
- If it is part of problem then the API grows with problem.senPool()
- If it is part of the factory, then the API grows even more with SensorFactory::get()->senPool()
I mean, if we want to use it directly in the createProcessor() call, then we want this call to be very concise and short, as in:
problem.createProcessor("FEATURE TRACKER", "Name", senPool("Camera Name"), createParams("tracker_params.yaml")) // only if both methods are static!That's why the question. Can I use
usingto shorten the call:using SensorFactory::senPool;or something similar?
Of course, the same question applies to createParams()...
-
Created by: nmansard
The "pool" (maybe several pools) is an object. I guess it is a singleton object. However, it may also be attached to the problem. Finally, it is a little bit the same, as the problem is also somehow a singleton object.
The methods to access the pool are doubtlessly member function of the pool object.
Note that, as soon as you start to manage objects with string ID, you need to have a pool. It might be more or less hidden, but it is there, somewhere.
Then yes, you can put the pool inside a namespace to store it, and derefer the namespace using "using" to simplify the code. Is it really interesting?
About createParam: I think the problem is a little bit different. It should be possible to have createParams() called exactly like createSensor().
-
Created by: joansola
My example of
using SensorFactory::senPooldoes not use a namespace. It is the object (in this example SensorFactory) and a member method (senPool) . Let me put it again with other names for clarity:class SensorPool{ SensorBase* quarySensor(const string & _name) { bla bla ; } } using SensorPool::querySensor;then I want to use
querySensor("Name")without specifying the objectSensorPool.The only interest is to have short names. It might be silly. I could also do
SenPool::get("Name")and it is short, too.But honestly, in my opinion, the pool is either in Problem, either in SensorFactory, thus not another isolated object!
-
Created by: joansola
OK, this is what I did to bind the processor to the sensor: 3 methods in Problem:
-
getSensorPtr("Name")returns asen_ptrgiven a name.nullptrif sensor not found. -
installProcessor("TYPE", "Name", sen_ptr, ¶ms)accepts asen_ptr -
installProcessor("TYPE", "Name", "Sen Name", ¶ms)accepts a sensor name.throwif sensor not found (maybe throwing is too severe, but we are at configuration time, it should be OK).
The method 3 is just a helper method calling 1, then 2. therefore, the string is not getting inside the API deeper than this 'helper' level.
The famous sensor pool is, basically, the list of sensors existing in the wolf tree. I have not replicated it with a
map<string, SensorBase*>or anything similar because I prefer having this info only once. This improves consistency. To query by name, I usefind_if.The code is in problem.h and problem.cpp, branch factory-auto-cpp.
-
Created by: joanvallve
Maybe, before taking those decisions, we should have the complete API design. At least, I miss
createCapture(or something like that).Its API, analogously to
createProcessor, will have the sensor name as input parameter, so thisgetSensorPtrfunctionality may be needed. However, it will be called each time a capture should be added.. right?-
Created by: joansola
CreateParams will have dedicated factories. Two things:
- There are params for Sensors, and params for Processors. As I used different factories for Processors and Sensors (because the API of create() is different), I'll probably use two factories for params, although now the API would be the same: Params* create("Type", "file.yaml")
- There might be different methods to parse our configuration files. I wonder if :
- my factory should accept them all, by registering 'translators' the same way it can register 'creators', and having
ParamsBase* create("TYPE", "FileName.any_type")
- or if I just make the factories for the YAML approach and leave the work of interfacing ROS:rosparams to other people, thus providing onlyParamsBase* create("TYPE", "File_Name_WOLF_style.yaml")
-
Created by: joansola
@joanvallve Yes, we need createCapture too. In this case, it's maybe easier, because in the sensor callback you can already know the sensor pointer for the whole lifetime of the program. You just need to retrieve it once, with problem.getSensorPointer("Sensor Name");
I agree that we should also put the Captures API in the discussion. Do you think there is something essentially different to what we are discussing about Sensors and Processors?
-
Created by: joanvallve
If we were designing an API for avoiding the final user to handle with wolf node pointers, we should keep designing createCapture in the same way... I propose something very similar to createProcessor but without need to give a name to the capture:
createCapture("type", "SensorName", data) -
Created by: joanvallve
This is the reason why I interfered in the discussion, the design of this query can be map based or a find... The order of the discussion should be: first deciding which API would be desirable, and then find a way to implement it. So, is there a way to change to a map based getSensor query?
-
Created by: joansola
@joanvallve There are two methods that I see:
- Change all lists<> in wolf into map<>, with a string key by name. Then, getSensor(string) is just a map<> query. This could be just an upgrade of NodeLinked, so it is not a crazy thing to think about. EDIT: yes, it's crazy because we'll have thousands of Features and Constraints and we'll need to generate unique strings to make them different.
- Replicate the sensor_ptr <--> "Sensor Name" bindings in a map<> in Problem, just for the queries we are talking about. Then, we need to be sure that what's in the map<> and what's in the list<> is the same!
-
Created by: joansola
Yes I agree map<> is not good in the general case of WOLF (see my EDIT before).
I think a pointer is the way to go to avoid these issues. It is simpler than replicating the list into a map, and safer. Even with a map<>, which is log(n), you'll have to compare strings, which is o(n) in the string length. I think it is unnecessary. Then, if one finds it practical, it's because the captures he is generating are not arriving at a high frequency. In such case, why bother about log(n) over a list of really a small number of sensors?
-
Created by: joansola
I implemented IntrinsicsLibrary, with YAML parser. An example that can be explored is working:
- IntrinsicsFactory.h / .cpp
- SensorCameralh / .cpp --> method createIntrinsics(), and registration at the bottom line of the .cpp.
- examples/test_wolf_factories.cpp --> line 51 !
To execute, you'll need to have libyaml-cpp installed.
-
Created by: joansola
I made the YAML things optional in WOLF. For this, I had to separate some of the things in the sensor_xxx.cpp files to new .cpp files in a yaml/ directory, and play with the CMAKE files to make it optional.
Things work really well, though the API still needs to converge a little further to something really practical. I hope we'll have the opportunity to keep this discussion alive.
You can checkout 'factory', then check and launch /test_wolf_factories.cpp!
A run now gives:
ProcessorFactory : registered IMU ProcessorFactory : registered ODOM 2D ProcessorFactory : registered ODOM 3D SensorFactory : registered CAMERA SensorFactory : registered GPS SensorFactory : registered GPS FIX SensorFactory : registered IMU SensorFactory : registered ODOM 2D ProcessorFactory : registered GPS IntrinsicsFactory : registered CAMERA ====== Registering creators in the Wolf Factories ====== If you look above, you see the registered creators. There is only one attempt per class, and it is successful! We do this by registering in the class's .cpp file. See [wolf]/src/examples/test_sensor_factory.cpp for the way to add sensors and processors to wolf::Problem. ================= Intrinsics Factory =================== --- Parameters Parsed from YAML file --- sensor type: CAMERA sensor name: Front left camera sensor extrinsics: position : 0 0 0.5 orientation : -1.5708 0 -1.5708 sensor intrinsics: image size : 1024 720 intrinsic : 516.686 355.129 991.852 995.269 distoriton : -0.301701 0.0963189 ==================== Sensor Factory ==================== Sensor 1 | type 5 : CAMERA | name: front left camera Sensor 2 | type 5 : CAMERA | name: front right camera Sensor 3 | type 1 : ODOM 2D | name: main odometer Sensor 4 | type 6 : GPS FIX | name: GPS fix Sensor 5 | type 4 : IMU | name: inertial Sensor 6 | type 7 : GPS | name: GPS raw Sensor 7 | type 1 : ODOM 2D | name: aux odometer Sensor 8 | type 5 : CAMERA | name: rear camera with intrinsics: 516.686 355.129 991.852 995.269 =================== Processor Factory =================== Processor 1 | type : ODOM 2D | name: main odometry | bound to sensor 3 : main odometer Processor 3 | type : IMU | name: pre-integrated | bound to sensor 5 : inertial Processor 2 | type : ODOM 3D | name: sec. odometry | bound to sensor 7 : aux odometer changed milestone to %Auto-configuration
added ~209 External API update labels and removed 1 deleted label
-
I'm no longer sure about the topic, so the status of this issue... @jsola, what do you think?
