Andrewi/97 canlink bringup #99
Conversation
|
Hey Andrew, I have lots of studying to do until Tuesday. I'll be sure to have a look at your PR then. |
Still need to add documentation for |
There was a problem hiding this comment.
I don't see where the Handler interface is, i.e. the pub-sub one which lets you attach behaviour to a CAN bus. The writers should implement this Handler interface. I don't think I should have to manually call writer.WriteFrameToFile. Shouldn't that be automatic when attached?
There was a problem hiding this comment.
Where do these functions get called? By go test or something? Just want to make sure that they should stay in the final PR
| ) | ||
|
|
||
| // Converts timestamped frames into strings for file writing | ||
| func convertToAscii(l *zap.Logger, timestampedFrame *TimestampedFrame) string { |
There was a problem hiding this comment.
this is pretty repetitive. Can you wrap the error handling in a local function to make it easier to read?
var builder strings.Builder
write := func(value string) {
if _, err := builder.WriteString(value); err != nil {
l.Error(err.Error())
}
}
write(timestampedFrame.Time.Format(_messageTimeFormat))
write(" " + strconv.FormatUint(uint64(timestampedFrame.Frame.ID), _decimal))
write(" Rx")
write(" " + strconv.FormatUint(uint64(timestampedFrame.Frame.Length), _decimal))
for i := uint8(0); i < timestampedFrame.Frame.Length; i++ {
write(" " + fmt.Sprintf("%02X", timestampedFrame.Frame.Data[i]))
}
return builder.String()this is from gpt, so make sure it's all good before using
| } | ||
|
|
||
| func (w *Writer) CreateTraceFile(traceDir string, busName string) error { | ||
| file, err := createEmptyTraceFile(traceDir, busName, w.fileExtention) |
There was a problem hiding this comment.
can you join these two lines with w.traceFile, err := create...?
There was a problem hiding this comment.
are the comments necessary in this file? The strings are self explanatory so I think the comments just risk becoming outdated
| } | ||
|
|
||
| func NewWriter(l *zap.Logger, fileExtention string) *Writer { | ||
| convertToStringMapping := map[string]func(*zap.Logger, *TimestampedFrame) string{ |
There was a problem hiding this comment.
this map prevents developers from defining their own converters since the only supported ones are json and ascii. (i'm thinking about how we want to make this available for others to use).
You could define an interface representing the Ascii and Json functions which include the to_string method and a .get_extension() -> str function. Then, you would pass one of those interfaces to NewWriter. This would make it possible for someone else to define a CSV converter and just pass it into NewWriter
| return errors.Wrap(err, "writing to trace file") | ||
| } | ||
|
|
||
| _, err = w.traceFile.WriteString("\n") |
There was a problem hiding this comment.
You could expect the .convertToString function to include the \n character. I think that makes more sense than adding it here.
#97 Added writer structs for json and ascii to handle file writing for tracer. Also added tests for both jsonWriter and asciiWriter. Changed tracer so that we write each frame to the trace file immediately after receiving the can frame through the socket. Added empty test for the tracer which just initializes the tracer, sleeps for 5 seconds, then closes the tracer. (Eventually we can add more robust testing for the tracer but just wanted to add this test for easy development. Just run the test then use cansend to ensure tracer is working)