Trislam1 #40
Trislam1 #40
Conversation
trislam/CMakeLists.txt
Outdated
| # -- Translate ruleset into CPP -- | ||
| translate_ruleset( | ||
| RULESET_FILE ${PROJECT_SOURCE_DIR}/gaia/slam.ruleset | ||
| DATABASE_NAME slam |
There was a problem hiding this comment.
There is no DATABASE_NAME parameter in translate_ruleset.
trislam/CMakeLists.txt
Outdated
| include("/opt/gaia/cmake/gaia.cmake") | ||
|
|
||
| # Default compiler/linker flags. | ||
| add_compile_options(-c -Wall -Wextra -ggdb) |
There was a problem hiding this comment.
This is redundant with set(GAIA_COMPILE_FLAGS "-c -Wall -Wextra -ggdb").
I'd remove the following:
set(GAIA_COMPILE_FLAGS "-c -Wall -Wextra -ggdb")
set(GAIA_LINK_FLAGS "-ggdb")
trislam/CMakeLists.txt
Outdated
| add_executable(test_analyze | ||
| tests/test_analyze.cpp | ||
| src/analyze.cpp | ||
| src/occupancy.cpp | ||
| src/paths.cpp | ||
| src/rule_api.cpp | ||
| src/utils/coord_list.cpp | ||
| src/utils/line_segment.cpp | ||
| ) | ||
| target_add_gaia_generated_sources(test_analyze) |
There was a problem hiding this comment.
It seems like we need to split slam_sim into a library and an executable re-use the library for tests.
trislam/CMakeLists.txt
Outdated
| set_target_properties(test_coord_list PROPERTIES COMPILE_FLAGS "${GAIA_COMPILE_FLAGS}") | ||
| set_target_properties(test_coord_list PROPERTIES LINK_FLAGS "${GAIA_LINK_FLAGS}") |
There was a problem hiding this comment.
I think you can get rid of all the set_target_properties declarations.
| @@ -0,0 +1,17 @@ | |||
|
|
|||
There was a problem hiding this comment.
empty line (mayve you should put the Gaia copyright)
There was a problem hiding this comment.
This file will be deleted. It's a shortcut to build ddl/rules w/o jumping back to cmake.
trislam/gaia/slam.ddl
Outdated
| ( | ||
| id uint32 unique, | ||
|
|
||
| ego references ego[], |
There was a problem hiding this comment.
By doing this the graphs table is the parent of the relationship. Should the ego be the parent of the relationship? (also because I can see an ego having multiple graphs but not a graph having multiple ego that by definition is a singleton table).
There was a problem hiding this comment.
Btw, we do support 1:1 non-VLR relationships. Since you should only have one ego this should be 1:1, correct? This will then clean up the generated code.
There was a problem hiding this comment.
This inclusion is to allow use of VLRs, as this is a reverse reference that can be deleted once we allow one-sided references.
Using a VLR here (defined in ego) allows the user to update one field in ego to update the relationship. Switching to explicit (non-VLR) makes the user worry about connecting and disconnecting, and which side of the record is the parent and which the child. VLR allows lower code and lower cognitive burden, at cost of using a syntax which is confusing here but will be eliminated once we have one-sided references
trislam/gaia/slam.ddl
Outdated
| -- The first includes tables that have individual records in them. For | ||
| -- example, the 'ego', the 'position', and maps that represent the | ||
| -- area (used for path finding). |
There was a problem hiding this comment.
I wonder if it would make sense (in the very long term) to have support for singleton databases objects. that would make querying (from DAC and declarative) much simpler. If you think it makes sense we can open a tracking JIRA.
A similar use case is support for constants (https://gaiaplatform.atlassian.net/browse/GAIAPLAT-1355)
trislam/gaia/slam.ddl
Outdated
| -- An edge connects two observations, source and destination. These | ||
| -- don't have to be directed but they can be. |
There was a problem hiding this comment.
Why they don't have to be directed? Isn't every observation monotonically increasing (time-wise) giving some sense of direction?
There was a problem hiding this comment.
A different way to put this is, why is this sentence relevant for the reader "These don't have to be directed but they can be."?
There was a problem hiding this comment.
Graph nodes are directed when the robot is following a path (A -> B -> C -> ...) but paths themselves can be intermixed, forming closures. For example, when making nodes M -> N -> O -> P the algorithm may determine that N and B should have an edge, and there's no intuitive or functional reason for a polarity in the direction of the N--B edge. I think we should get rid of the idea of polarity in edges altogether (but I haven't pulled the trigger for executing that idea yet)
trislam/gaia/slam.ddl
Outdated
| -- from that optimization, but more relevantly creating this record also | ||
| -- generates as an event to do post-optimization things, like creating a | ||
| -- new map. | ||
| table error_corrections |
There was a problem hiding this comment.
What is the nature of the corrections? (eg. swapping a vertex in the graph, changing the constraints of some edge, etc...)
There was a problem hiding this comment.
The overall goal is to alter edge lengths (see intro SLAM video) thus adjusting the coordinates of the nodes. How that's implemented involves mathematical voodoo. It's foreseeable that there's some state to save that goes with that, hence having this a potential data-storing record rather than simple event (it can always be downgraded to being only an event)
| in_edges references edges[], | ||
| out_edges references edges[], | ||
|
|
||
| prev_observation references latest_observation[] |
There was a problem hiding this comment.
should this be a 1:1 relationship? (yeah, but then we can;t do VLR, probably we should remove this constraint)
There was a problem hiding this comment.
Well, if we're not doing VLR here and it should be 1:1 then let's make it 1:1.
There was a problem hiding this comment.
This is VLR. It's a reverse reference that's not used. Once we have one-sided references we can delete it. I'd rather not switch from VLR to explicit connect/disconnect because of limitations on our reference model that make some reference uses awkward (something that we plan to change/fix)
trislam/gaia/slam.ddl
Outdated
| top_meters float, | ||
| bottom_meters float, | ||
|
|
||
| ---------------------------- |
There was a problem hiding this comment.
nit: I feel like a blank line is sufficient to delineate table fields from references. Also, it looks like you don't consistently use --------- in your tables.
There was a problem hiding this comment.
Guilty as charged re: consistency. Will look for consistent style. Having only blank lines turns the sea of text into a jumble to my eyes, hence efforts to break it up.
trislam/gaia/slam.ddl
Outdated
| ) | ||
|
|
||
|
|
||
| -- Link to most recently created observation. If multiple observations are |
There was a problem hiding this comment.
nit: confusing wording since we call the name of the reference inside a table a link. I would just say this this is "Represents the most recently created observation".
trislam/gaia/slam.ddl
Outdated
|
|
||
|
|
||
| ------------------------------------------------------------------------ | ||
| -- Data tables |
There was a problem hiding this comment.
nit: since the first section is for "single-record" tables maybe this should be labeled 'multi-record' tables. Just sayin': one record is "data", too :).
trislam/gaia/slam.ddl
Outdated
| graph_id uint32 | ||
|
|
||
| -- A reference can be made to graphs if necesasry. However, if it's | ||
| -- unlikely to be used, or be used infrequently, might be better |
There was a problem hiding this comment.
Nitty McNitNit: is the extra space on line 120-123 intentional? I think it would be just as readable if it matched the spacing on line 119.
There was a problem hiding this comment.
It's a coding habit of using to indent subsequent lines in a comment block (as with a code block). It also keeps "paragraphs" together :-) Can revert if it's distracting.
trislam/gaia/slam.ddl
Outdated
| table observations | ||
| ( | ||
| ---------------------------------------------- | ||
| -- Static data. These values are set at creation and don't change. |
There was a problem hiding this comment.
If the static portion of observations is truly static then consider the following alternatives:
- Don't store static "read-only" data in gaia at all. There is no need for a transaction to read this data at all.
- Store in a separate table, then load into a local data structure at init time.
- Keep in this table, load once into a local data struct at init time. (Maybe this is what you are doing)
There was a problem hiding this comment.
Perhaps 'static' is not the right word? (I'm thinking engineering terminology here, not computer science). Having field values in a record that won't change is normal. What I'm trying to say here is to help differentiate what stays constant in a record, and thus cannot be involved in a transaction conflict, and those values that do change and hence can create a conflict.
Open to suggestions!!
trislam/gaia/slam.ddl
Outdated
| ( | ||
| -- The latest best guess of position. It may be modified in the future | ||
| -- when we have better error estimates. | ||
| pos_x_meters float, |
There was a problem hiding this comment.
nit: positions.pos_x_meters seems redundant. Why not just: positions.x_meters?
trislam/gaia/slam.ddl
Outdated
|
|
||
| table movements | ||
| ( | ||
| -- DR motion from previous observation. This is a placeholder for storing |
trislam/gaia/slam.ruleset
Outdated
| } | ||
| } | ||
|
|
||
| on_insert(o: observations) |
There was a problem hiding this comment.
This rule intentionally left blank?
|
|
||
| ruleset observation_ruleset | ||
| { | ||
| on_insert(e: edges) |
There was a problem hiding this comment.
I still prefer not to use tags in situations where they don't enhance readability. Readers might believe they are necessary; they are not in many situations.
There was a problem hiding this comment.
This is a great style/best-practices question. I'm partial to using full paths to declare a variable as it reduces some possibility for errors. It's helpful to be able to see what fields are being referenced (helpful as a schema gets more complex). Again, just a style issue though. Something we need to solidify our story on.
Auxiliary thought: if we encourage using field names that auto-resolve then we should encourage using unique names for all fields regardless of table, for style consistency.
There was a problem hiding this comment.
Tags represent the current row of a table, not a field value. Stylistically, I'm inclined towards using table.field in rules. If you have different rows of the same table, then tags are necessary to understand which row you are talking about (anchor row, for example, versus say a row you got from looking globally). In this rule, using e instead of edges is adding no semantic value.
| .where(graphs_t::expr::id == ec.graph_id).begin()); | ||
| build_map(g, /observed_area); | ||
| } | ||
| } |
There was a problem hiding this comment.
I had thought there would be more rules than this. Looking at this ruleset makes me wonder ... why use Gaia at all if this is all the benefit I get? Are more rules coming and you just wanted initial feedback or is this how this is going?
There was a problem hiding this comment.
(I will say that in original Clearpath discussions I heard they were most interested in our database so perhaps this minimal ruleset does adequately reflect their use-case.)
trislam/include/coord_list.hpp
Outdated
|
|
||
| struct map_coord_t | ||
| { | ||
| double x_meters; |
There was a problem hiding this comment.
May not be an issue but I see you are using double precision here but float in the DDL tables. Is this intentional?
trislam/include/coord_list.hpp
Outdated
| class coord_list_t : public std::vector<map_coord_t> | ||
| { | ||
| public: | ||
| coord_list_t(std::string file_name); |
There was a problem hiding this comment.
nit: this can be a reference (std::string& file_name)
trislam/include/line_segment.hpp
Outdated
| namespace utils | ||
| { | ||
|
|
||
| constexpr double R2D = 180.0 / M_PI; |
There was a problem hiding this comment.
nit: c_rad_to_deg might be more readable and not make folks think that the constant is a macro.
|
|
||
| struct sensor_data_t | ||
| { | ||
| uint32_t num_radials; |
There was a problem hiding this comment.
Can sensor data be stored in Gaia? (if not persisted, at least managed as a table that can be triggered when a new senser reading comes in)?
There was a problem hiding this comment.
It can be, but that's not consistent w/ the ClearPath use case right now. The data seems to come in a stream on a lower level (i.e., ROS) and something at that level examines the state of the system and determines if it's worthy to create a new node/observation/pose using that sensor data, in which case it does.
Data is too extensive for all sensor data to persist (they have 1GB database files using the filtering they're doing).
Pushing the latest stream data into the table and having that trigger a rule to analyze it is indeed possible. The benefit for doing so isn't immediately clear though (not to say that there might not be improvements somewhere, so long as races are protected against using a serial_group). It's certainly worth suggesting
|
|
||
|
|
||
| //////////////////////////////////////////////// | ||
| // Rules API |
There was a problem hiding this comment.
clarification: specifically, these functions are invoked by the rules in slam.ruleset.
| // Rules API | ||
|
|
||
| // Determines if it's time to perform a graph optimization. | ||
| bool optimization_required(); |
There was a problem hiding this comment.
Could this logic be encoded as a rule? Looking at the code, you are saying "force an optimization every X observations". This seems like it would map t a rule that increments the counter whenever an observation is inserted. (currently the rule is blank).
There was a problem hiding this comment.
It is an API called from a rule so it could certainly be inlined there. The implementation is just a placeholder though (and a pretty bogus one at that). I tried to keep the schema/rules w/o an arbitrary implementation that might be distracting (but if you feel otherwise that can be changed)
I also left it as a separate function so that the logic of the rule is easier to read (some say rules should be extremely short, some say they should have all logic in them, some say...!!)
|
|
||
| void load_world_map(const char* world_map) | ||
| { | ||
| FILE* fp = fopen(world_map, "r"); |
There was a problem hiding this comment.
If Clearpath is using C++17 then they can use the C++ Filesystem library.
There was a problem hiding this comment.
D'oh. My C background keeps shining through ;-)
Will look at C++ifying
| void occupancy_grid_t::export_as_pnm(string file_name) | ||
| { | ||
| uint32_t n_pix = m_grid_size.cols * m_grid_size.rows; | ||
| uint8_t* r = new uint8_t[n_pix]; |
There was a problem hiding this comment.
You could use unique_ptr (which does work for arrays). Benefit is that you don't need the delete[] at exit and the dtor will get called no matter how you exit the function (early return, exception, etc).
trislam/src/paths.cpp
Outdated
| // transaction | ||
|
|
||
|
|
||
| static void update_observed_area(ego_t& ego, map_coord_t coord) |
There was a problem hiding this comment.
Not called from a rule currently, TBD?
trislam/src/paths.cpp
Outdated
| // Left and right bounds. | ||
| float left_edge = area.left_meters(); | ||
| float right_edge = area.right_meters(); | ||
| printf("OBSERVED AREA at %.2f,%.2f\n", coord.x_meters, coord.y_meters); |
There was a problem hiding this comment.
Would be nicer (I think) to use gaia_log functionality (app logger) here and then you can use the trace levels (info, debug, error, warning, etc).
trislam/src/paths.cpp
Outdated
| using utils::sensor_data_t; | ||
|
|
||
| //////////////////////////////////////////////////////////////////////// | ||
| // Rule API |
There was a problem hiding this comment.
Maybe this is just an old comment? You have another module rule_api.cpp that also has code called from rules.
There was a problem hiding this comment.
@simone-gaia did something cool in the mqtt example where cmake pulls down the json.hpp file. https://github.com/gaia-platform/examples-internal/blob/main/mqtt/CMakeLists.txt
I'm incorporating that into the changes I'm making to access_control. I think it would be good to do that here as well.
trislam/Makefile
Outdated
|
|
||
| all: | ||
| mkdir -p build && cd build && cmake .. && make -j 4 | ||
|
|
||
| run: | ||
| cd src && make run | ||
|
|
||
| # At present there's no method to delete the contents of the database | ||
| # from w/in the program, so add a way to do so outside. | ||
| refresh_db: | ||
| gaiac gaia/slam.ddl --db-name slam -g -o /tmp/slam | ||
|
|
||
| refresh: clean all | ||
|
|
||
| clean: | ||
| rm -Rf build | ||
|
|
There was a problem hiding this comment.
I see what you are doing here, and it makes sense to me, but AFAIK this is the only project where we do this. I know that we had a bunch of shell scripts in the AMR swarm, but the jury was out on whether they helped or caused more confusion. I prefer this to shell scripts, but I also wonder if there is a way to generalize this so that we could just drop a copy of this in with all the examples? The only project specific rule is refresh_db.
What do other folks think?
| { | ||
| y_idx = m_grid_size.rows - 1; | ||
| } | ||
| //printf(" %.2f,%.2f at idcs %d,%d (of %d,%d = %d)\n", x_meters, y_meters, x_idx, y_idx, m_grid_size.cols, m_grid_size.rows, x_idx+y_idx*m_grid_size.cols); |
There was a problem hiding this comment.
Let's remove this (and other similar ones), or replace them with a debug log output.
| -- Tables with single records (i.e., exactly one) | ||
|
|
||
| -- | ||
| table ego |
There was a problem hiding this comment.
Assuming this is carried over from the GaiaBot example which I believe @dr-soong used (at least partially) as a starting point. For GaiaBot I had a recurring timed event to do periodic calculations and I found that an entry in an ego table (a.k.a. a singleton database object) was effective for navigating through references to access the data necessary for the calculations. It appears that Keith is doing something similar but from the direct access code. Right Keith?
Migration of 'slam_sim' to a ClearPath focused example. This branch represents a work in progress, but at this point is mostly operational.