farm-ng / farm-ng-core Goto Github PK

    Not sure why I have this file in Pangolin - it isn't used and I think you can omit it.

Originally posted by @stevenlovegrove in #52 (comment)

Currently, any format error such as FMT("arg1: {}, arg2:P{}", 5); will (most likely) crash the program. This is in most cases not desired. It will turn a minor documentation error into program abortion.

Proposal:

• Log a warning with the raw format string e.g. "arg1: {}, arg2:P{}" plus the exception message and then continue execution as normal.

Alternative:

• Re-enable compile time fmt string parsing,. However this might involve sig. engineering work and also increase compile time.

    @Hackerman342 @ethanrublee i have a proposal here to improve the current api and make it more user friend.

Random access here might be error prone, we could make it via dictionary and use the key as the semantics of the uri.

uris: Dict[str, uri_pb2.Uri] = reader.uris()
uri: uri_pb2.Uri = uris["oak0/video"]

Originally posted by @edgarriba in farm-ng/farm-ng-amiga#31 (comment)

As a reader of logs, i'd like to depend on event.stamps[-1] always being the monotonic stamp, as this makes it easy/cheap to sort the events by write order without searching for the stamp by clock name.

Known missing tests:

• utils/erase_test.cpp
• utils/stopwatch_test.cpp
• utils/tokenize_test.cpp
• utils/thread_pool_test.cpp
• utils/time_test.cpp
• Check logging for additional missing tests
• Check utils for additional missing tests
• Check pipeline for additional missing tests

There's been some questions on the team about what our C++ conventions are around the use of the PIMPL pattern, which seems like a pattern we could offer guidance about in our C++ style guide.

To get the conversation started, here are a few statements:

• We currently make regular use of a pattern that looks like PIMPL -- it hides implementation, with some of the same compilation firewall benefits as PIMPL, but it is not strictly-speaking PIMPL. It requires less boilerplate than PIMPL, since we don't need to provide a duplicate internal method that wraps each public method.
• There is a great definition of the pattern we use (interface + factory) and a comparison with PIMPL here.
• We have a type Shared that is useful for enforcing that a shared pointer is not null. By convention the factories in the pattern above return this Shared type.

And a few suggestions:

• We should document the motivation for this pattern and offer guidance when it should be applied.
• Our factories should return unique_ptrs (or a non-null Unique) rather than Shared for the reasons outlined here.
• We could adopt GSL's not_null for a battle-tested implementation of not-null semantics that would apply to all raw and smart pointer types.

template<typename T>
using Unique = gsl::not_null<std::unique_ptr<T>>;

    @Hackerman342 @ethanrublee an improvement here is to merge `get_event` and `read_message` since we use all the time together. For testing and some internal use cases is good but better coupled for a more user friendly api

def read_uri_message(self, uri: uri_pb2.Uri, event_id: int) -> Any:
    # get the event and offset
    event: event_pb2.Event
    offset: int
    event, offset = self.get_event(uri, current_event)

    # seek and parse the message
    return self.read_message(event, offset)

maybe read_message should be renamed to parse_message and read_message recive the uri and event_id

Originally posted by @edgarriba in farm-ng/farm-ng-amiga#31 (comment)

review / design behavior of ASSERT macros when compiled in RELEASE mode

options

• turn then all off
• turn them into warnings (no aborts)
• leave them as is, but introduce DASSERT macros which only fire in DEBUG or in DEBUG+RelWithDebInfo mode
• etc.

There is no plan to chnge behavior for ASSERTs in DEBUG and RelWithDebInfo modes.

As future feature this could be part of the EventWriter that takes as input the file base name and max length and internally handle the spliting. Same for the reader, to handle multiple files from a sequence as one. Actually, with this proposal, it will save several of files which can be a bit difficult to track in the gallery/playback app. Maybe we need to append to the parent files messages pointing to the next file ?

Originally posted by @edgarriba in https://github.com/farm-ng/workspace/issues/1084#issuecomment-1373580359

Add a small utility in the Python API that the allows to filter the events index by types

def event_has_message(event: event_pb2.Event, msg_type: Any) -> bool:
    """Check if event has a message of type msg_type.

    Args:
        event: event_pb2.Event
        msg_type: protobuf message type

    Returns:
        bool: True if event has a message of type msg_type
    """
    return event.uri.query.split("&")[0].split(".")[-1] == msg_type.__name__

Currently (v1.0.0-dev) the pip build for macos-latest fails. This is relevant because we want to be able to build this package so macOS users can pip install farm-ng-core again.

    I think we should scrap these operator+ methods. I haven't been using them as I think they're a bit confusing.

Originally posted by @stevenlovegrove in #52 (comment)

Initial PR merging sophus2 and farm-ng-core in: #105

• sort cpp/sophus2 better into existing directory structure
  #113
• docs:
  • revive/merge docs
• review all scripts in top level folder and see whether they are still needed
• dependency management
  • document venv
    #113
  • merge / simply venv build scripts
    #113
  • merge Eigen/format submodules with venv build process
• cmake
  • flatten directory structure
  • merge cmake/scripts with infra/scripts
• CI tooling
  • revive clang-tidy
  • asan
  • compile time analysis
• Merge geometric protos in sophus namespace with newly introduced protos in farm_ng namespace.
  • This will break log formats in downstream projects and should be done on a as needed basis only.
• use specialization instead of ADL for proto conversion
  to enable more general purpose event logger and playback tools (e.g. such tool do not need to include all proto_conv headers, but only the definition of the toProto / fromProto function template)
  • #135