Conversation
…are needed to complete a block/SOF
… the refactored pixy_parser file
… the output vals(x,y,w,h)
… statements caused sem block)
…river completed. Ready for Live testing
…are needed to complete a block/SOF
… the refactored pixy_parser file
… the output vals(x,y,w,h)
… statements caused sem block)
…river completed. Ready for Live testing
tridge
left a comment
tridge
left a comment
There was a problem hiding this comment.
quite difficult to review unfortunately. I've done a first pass, but really needs some cleanup to make the code clearer
| } | ||
| } | ||
|
|
||
| bool AC_PrecLand::target_acquired() |
There was a problem hiding this comment.
it's best not to change formatting for no reason. Also please follow the ArduPilot style guide.
http://ardupilot.org/dev/docs/style-guide.html
| return _target_acquired; | ||
| } | ||
|
|
||
| void AC_PrecLand::set_target_acquired(bool _target_acquired_value) { |
There was a problem hiding this comment.
why use an _ prefix on the variable name for a function?
(some people like _ for private variables in classes, which is fine if you like it, but pointless for local variables)
| public: | ||
| // Constructor | ||
| AC_PrecLand_Backend(const AC_PrecLand& frontend, AC_PrecLand::precland_state& state) : | ||
| AC_PrecLand_Backend(AC_PrecLand& frontend, AC_PrecLand::precland_state& state) : |
There was a problem hiding this comment.
why remove the const? Better to expose the data you want in the state struct
|
|
||
| protected: | ||
| const AC_PrecLand& _frontend; // reference to precision landing front end | ||
| AC_PrecLand& _frontend; // reference to precision landing front end |
| irlock(), | ||
| _have_los_meas(false), | ||
| _los_meas_time_ms(0) | ||
| _los_meas_time_ms(0), |
There was a problem hiding this comment.
we don't actually need to initialise values to 0 or false, as it happens automatically (as we override memory allocation with a function that zeros memory)
| } irlock_target_info; | ||
|
|
||
| irlock_target_info _target_info; | ||
| irlock_target_info _target_info[10]; |
There was a problem hiding this comment.
this looks like the source for the magic 10 above
|
|
||
| irlock_target_info _target_info; | ||
| irlock_target_info _target_info[10]; | ||
| size_t _num_targets; |
| You should have received a copy of the GNU General Public License | ||
| along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
| */ | ||
|
|
There was a problem hiding this comment.
is this parser code from another project, or is it new code?
If from an existing project then we should refer to that project and give credit
| #include <string.h> | ||
|
|
||
| #define PIXY_PARSER_PIXY_BUF_SIZE 17 | ||
| #define PIXY_PARSER_MAX_BLOBS 10 |
There was a problem hiding this comment.
maybe this is the magic 10?
|
|
||
| if (validity == MESSAGE_INVALID) { // If message invalid, wait and read 2 bytes | ||
| pixy_len--; ///----------------------------------------QUES---------------------------------------------- | ||
| memmove(pixy_buf, pixy_buf+1, pixy_len); // This discards the first block of the pixy_buf and makes pixy_buf have everything except the fisrt block value |
There was a problem hiding this comment.
i don't understand how this works. Looks like a byte array, so this shifts by one byte, but comment implies its a block?
3 participants
No description provided.