Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Console display refactoring #11

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,5 @@ morserino_display
dist/*
notes/help.txt
cover.out
coverage.txt
coverage.txt
*.log
23 changes: 14 additions & 9 deletions cmd/console.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,28 +22,33 @@ THE SOFTWARE.
package cmd

import (
"github.com/on4kjm/morserino_display/pkg/morserino_core"
"fmt"
"os"

"github.com/on4kjm/morserino_display/pkg/morserino"
"github.com/spf13/cobra"
)

// consoleCmd represents the console command
var consoleCmd = &cobra.Command{
Use: "console",
Short: "A brief description of your command",
Long: `A longer description that spans multiple lines and likely contains examples
and usage of using your command. For example:

Cobra is a CLI library for Go that empowers applications.
This application is a tool to generate the needed files
to quickly create a Cobra application.`,
Short: "Displays the Morserino output to the console",
Run: func(cmd *cobra.Command, args []string) {
morserino_core.Morserino_console(morserinoPortName)
err := morserino.SetupLogger(morserinoDebugLevel, morserinoDebugFilename)
if err != nil {
fmt.Println("\nERROR: " + err.Error() + "\n")
cmd.Help()
os.Exit(1)
}
Comment on lines +37 to +42
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nit: you can inline this.
Also Println:

  • already adds a /n at the end of the line.
  • Joins all given arguments with a whitespace character.
Suggested change
err := morserino.SetupLogger(morserinoDebugLevel, morserinoDebugFilename)
if err != nil {
fmt.Println("\nERROR: " + err.Error() + "\n")
cmd.Help()
os.Exit(1)
}
if err := morserino.SetupLogger(morserinoDebugLevel, morserinoDebugFilename); err != nil {
fmt.Println("\nERROR:", err)
cmd.Help()
os.Exit(1)
}

morserino.Morserino_console(morserinoPortName)
},
}

func init() {
rootCmd.AddCommand(consoleCmd)

//FIXME: the port parameter should be moved here

// Here you will define your flags and configuration settings.

// Cobra supports Persistent Flags which will work for this command
Expand Down
13 changes: 11 additions & 2 deletions cmd/list.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,10 @@ THE SOFTWARE.
package cmd

import (
"github.com/on4kjm/morserino_display/pkg/morserino_core"
"fmt"
"os"

"github.com/on4kjm/morserino_display/pkg/morserino"

"github.com/spf13/cobra"
)
Expand All @@ -34,7 +37,13 @@ var listCmd = &cobra.Command{
Long: `Displays the ports available on the system.
`,
Run: func(cmd *cobra.Command, args []string) {
morserino_core.Morserino_list()
err := morserino.SetupLogger(morserinoDebugLevel, morserinoDebugFilename)
if err != nil {
fmt.Println("\nERROR: " + err.Error() + "\n")
cmd.Help()
os.Exit(1)
}
morserino.Morserino_list()
},
}

Expand Down
13 changes: 9 additions & 4 deletions cmd/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ var cfgFile string
//Global variable with the port name
var morserinoPortName string

//Global variable with the debug level
var morserinoDebugLevel string

//Global variable with the name of the log file (if defined)
var morserinoDebugFilename string

Comment on lines +40 to +45
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • You can use the multi-line var syntax here, which allows to group symbols together, preferably semantically.
  • Also, I don't see why we need to repeat morserino here. It doesn't bring much information, does it ?
  • What does debugLevel means ? Shouldn't it be logLevel ?
  • Those variable names are pretty self descripting. The comments only repeat what is already stated by the variable. I suggest you remove them?
Suggested change
//Global variable with the debug level
var morserinoDebugLevel string
//Global variable with the name of the log file (if defined)
var morserinoDebugFilename string
var (
//Global variable with the debug level
debugLevel string
//Global variable with the name of the log file (if defined)
debugFilename string
)

// rootCmd represents the base command when called without any subcommands
var rootCmd = &cobra.Command{
Use: "morserino_display",
Expand All @@ -51,7 +57,7 @@ For easy reading, a new line is inserted after a "=" sign.
// Run: func(cmd *cobra.Command, args []string) { },
// Run: func(cmd *cobra.Command, args []string) {
// //Displaying on console is de default behaviour
// morserino_com.ConsoleListen(morserinoPortName)
// morserino_com.Listen(morserinoPortName)
// },
}

Expand All @@ -69,12 +75,11 @@ func init() {
// will be global for your application.

rootCmd.PersistentFlags().StringVar(&morserinoPortName, "port", "auto", "Morserino port (if set to \"auto\", will try to identify the port)")
rootCmd.PersistentFlags().StringVar(&morserinoDebugLevel, "debug", "", "Activtes debug tracing and its level (\"info\", \"debug\", \"trace\")")
rootCmd.PersistentFlags().StringVar(&morserinoDebugFilename, "logfile", "", "Specifices a specific filename to collect the debug info (\"stdout\" is a valid filename")

rootCmd.PersistentFlags().StringVar(&cfgFile, "config", "", "config file (default is $HOME/.morserino_display.yaml)")

// Cobra also supports local flags, which will only run
// when this action is called directly.
rootCmd.Flags().BoolP("toggle", "t", false, "Help message for toggle")
}

// initConfig reads in config file and ENV variables if set.
Expand Down
6 changes: 4 additions & 2 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@ go 1.16

require (
github.com/mitchellh/go-homedir v1.1.0
github.com/rs/zerolog v1.23.0
github.com/spf13/cobra v1.1.3
github.com/spf13/viper v1.7.1
github.com/spf13/viper v1.8.0
github.com/stretchr/testify v1.7.0
github.com/tidwall/gjson v1.8.0
go.bug.st/serial v1.1.3
golang.org/x/sys v0.0.0-20200909081042-eff7692f9009
golang.org/x/sys v0.0.0-20210616094352-59db8d763f22
)
397 changes: 376 additions & 21 deletions go.sum

Large diffs are not rendered by default.

Binary file added notes/img/Morserino - architecture.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added notes/img/morserino - happy case.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
18 changes: 17 additions & 1 deletion notes/notes.md
Original file line number Diff line number Diff line change
Expand Up @@ -61,5 +61,21 @@ See also [Create a CLI in golang with Cobra](https://codesource.io/create-a-cli-
* [Think Differently About What to Log in Go: Best Practices Examined](https://www.loggly.com/blog/think-differently-about-what-to-log-in-go-best-practices-examined/)
* [How to Design a Basic Logging System in Your Go Application](https://betterprogramming.pub/understanding-and-designing-logging-system-in-go-application-c85a28bb8526)

## Go Routines and channels
### Stopping
* [Stopping Goroutines](https://medium.com/@matryer/stopping-goroutines-golang-1bf28799c1cb), 2015
* [How to kill execution of goroutine?](https://www.golangprograms.com/how-to-kill-execution-of-goroutine.html)
* [Go - graceful shutdown of worker goroutines](https://callistaenterprise.se/blogg/teknik/2019/10/05/go-worker-cancellation/), 2019
* [Stopping goroutines](https://riptutorial.com/go/example/6055/stopping-goroutines)
* [Never start a goroutine without knowing how it will stop](https://dave.cheney.net/2016/12/22/never-start-a-goroutine-without-knowing-how-it-will-stop), Dave Cheney, 2016

### Channels and error handling
* ["Goroutines Error Handling"](https://www.atatus.com/blog/goroutines-error-handling/), blog, 2015
* ["Go Concurrency Patterns: Pipelines and cancellation"](https://blog.golang.org/pipelines), blog.golang.org, 2014
* ["Pipeline Patterns in Go: Pipelines with Error-handling and Cancellation"](https://medium.com/statuscode/pipeline-patterns-in-go-a37bb3a7e61d), medium, 2017
* ["Idiomatic goroutine termination and error handling"](https://stackoverflow.com/questions/40809504/idiomatic-goroutine-termination-and-error-handling), StackOverflow, 2016
* ["Return error from the channel"](https://stackoverflow.com/questions/25142016/return-error-from-the-channel), StackOverflow, 2014
* ["Capture Output and Errors of Goroutine Using Channels"](https://stackoverflow.com/questions/48981528/capture-output-and-errors-of-goroutine-using-channels), StackOverflow, 2018

## Miscelaneous
* [Buy Me a Coffe badge](https://gist.github.com/gbraad/216f8162d9b382d14b8a097a37cc2c72#file-readme-md)
* [Buy Me a Coffee badge](https://gist.github.com/gbraad/216f8162d9b382d14b8a097a37cc2c72#file-readme-md)
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package morserino_console
package morserino

/*
Copyright © 2021 Jean-Marc Meessen, ON4KJM <[email protected]>
Expand All @@ -22,30 +22,25 @@ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
THE SOFTWARE.
*/

import (
"fmt"
"strings"
)
// Structure containing all the channels used by the application
type MorserinoChannels struct {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Design nit: What is this structure role? What is its responsibility in regard of your application logic?
Usually we try to have one structure, one behavior. Here it seems that this structure is a technical factorization, not a semantic one which isn't the best.

Alsoooo, is repeating Morserino really necesasry here? We're already under the morserino package 😄

// Channel for the data flow from serial port to display goroutines
MessageBuffer chan string

//FIXME: Add comment
type ConsoleDisplay struct {
currentLine string
newLine string
}
// Channel indicating that all data has been displayed and that the
// application can be closed
DisplayCompleted chan bool
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
DisplayCompleted chan bool
DisplayCompleted chan struct{}

If you don't care about the actual data carried inside the channel, use chan struct{} instead (literally chan of empty struct} which doesn't allocate any space for the data.

You can use it like this

c := make(chan struct{})
c <- struct{}{}


func (cd *ConsoleDisplay) String() string {
//FIXME: add something useful here
return ""
}
Done chan bool

func (cd *ConsoleDisplay) Add(buff string) {
if strings.Contains(buff, "=") {
//FIXME: is the buffer one char long
fmt.Println("=")
} else {
fmt.Printf("%s", buff)
}
// Channel used to report back errors in goroutines
Error chan error
}

//TODO: add break on column
//TODO: Add tests
//Initialize the channels
func (mc *MorserinoChannels) Init() {
mc.MessageBuffer = make(chan string, 10)
mc.DisplayCompleted = make(chan bool)
mc.Done = make(chan bool)
mc.Error = make(chan error, 10)
}
Comment on lines +41 to +46
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Prefer using Initialization function instead of Init method: doing the later exposes you to the risk of manipullating sturctures in an ill state (and it makes your structure more complicated to manipulate from the caller).

var c MorserinoChannels
// c is in an ill state, if I call anything on it it will panic
c.Init()
// now we're good

VS

cs := NewMorserinoChannels()
// And we're good :tada:, no risk of panics here.

Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package morserino_com
package morserino

/*
Copyright © 2021 Jean-Marc Meessen, ON4KJM <[email protected]>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package morserino_com
package morserino

/*
Copyright © 2021 Jean-Marc Meessen, ON4KJM <[email protected]>
Expand Down
66 changes: 48 additions & 18 deletions pkg/morserino_com/listen_com.go → pkg/morserino/com_listen.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package morserino_com
package morserino

/*
Copyright © 2021 Jean-Marc Meessen, ON4KJM <[email protected]>
Expand All @@ -25,33 +25,41 @@ THE SOFTWARE.
import (
"fmt"
"io"
"log"
"strings"
"testing/iotest"

"github.com/on4kjm/morserino_display/pkg/morserino_console"
// "github.com/rs/zerolog"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Careful with that commented line 😄

"go.bug.st/serial"
)

const exitString string = "\nExiting...\n"

// Main listen function with display to the console
func ConsoleListen(morserinoPortName string, genericEnumPorts comPortEnumerator) error {
func OpenAndListen(morserinoPortName string, genericEnumPorts comPortEnumerator, channels *MorserinoChannels) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not in fond of this change: You're removing an error return and instead push down a set of channels that makes the code more complicated to understand.

How about keeping your error return and run the OpenAndListen in an anonymous function like this:

var errCh = make(chan error)

// ...

go func() {
         if err := OpenAndListen(foo, bar); err != nil {
              errCh <- err
         }
}() 

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, most of your operations here are initialization phase error (aka misconfigured application for example) , not properly runtime. Those operations should be handled synchronously in your main goroutine I believe.

Maybe leverage a struct here, WDYT ?

type Listenser struct {

}

// Initialization function for the initialization phase 
func NewListener(cfg Config) (*Listener, error) {
     // Initialize things that could fail synchronously
     smth, err := initializeSomething(cfg);
     if err != nil {
         return nil, err
     }
     
     return &Listener{smth: smth}, nil 
}

Another "smell" that makes me think this is your function name OpenAndListen: you're doing two things in the same function that could be separated.

Does it make sense ?


//If requested, use the simulator instead of a real Morserino
if strings.HasPrefix("SIMULATOR", strings.ToUpper(morserinoPortName)) {
AppLogger.Debug().Msg("Simulator mode listener")
TestMessage := "cq cq de on4kjm on4kjm = tks fer call om = ur rst 599 = hw? \n73 de on4kjm = <sk> e e"
return Listen(iotest.OneByteReader(strings.NewReader(TestMessage)))
err := Listen(iotest.OneByteReader(strings.NewReader(TestMessage)), channels)
channels.Error <- err
return
}

//If portname "auto" was specified, we scan for the Morserino port
if strings.ToUpper(morserinoPortName) == "AUTO" {
AppLogger.Debug().Msg("Tying to detect the morsorino port")
portName, err := DetectDevice(genericEnumPorts)
channels.Error <- err
if err != nil {
return err
channels.MessageBuffer <- exitString
channels.Done <- true
return
}
morserinoPortName = portName
}

log.Println("Listening to port \"" + morserinoPortName + "\"")
AppLogger.Info().Msg("Listening to port \"" + morserinoPortName + "\"")

//Port parameters for a Morserino
mode := &serial.Mode{
Expand All @@ -61,20 +69,25 @@ func ConsoleListen(morserinoPortName string, genericEnumPorts comPortEnumerator)
StopBits: serial.OneStopBit,
}

AppLogger.Debug().Msg("Trying to open " + morserinoPortName)
p, err := serial.Open(morserinoPortName, mode)
if err != nil {
return err
AppLogger.Error().Err(err).Msg("Error opening port")
channels.Error <- err
return
}
defer p.Close()

return Listen(p)
err = Listen(p, channels)
channels.Error <- err
return
}

// Main receive loop
func Listen(port io.Reader) error {
func Listen(port io.Reader, channels *MorserinoChannels) error {

//TODO: needs to be moved as a goroutine
consoleDisplay := morserino_console.ConsoleDisplay{}
// //TODO: needs to be moved as a goroutine
// consoleDisplay := morserino_console.ConsoleDisplay{}

// variables for tracking the exit pattern
var (
Expand All @@ -88,7 +101,7 @@ func Listen(port io.Reader) error {
// Reads up to 100 bytes
n, err := port.Read(buff)
if err != nil {
log.Fatal(err)
AppLogger.Error().Err(err).Msg("Error reading on port")
}

// Check whether the "end of transmission" was sent
Expand All @@ -110,23 +123,40 @@ func Listen(port io.Reader) error {
}

if n == 0 {
fmt.Println("\nEOF")
AppLogger.Debug().Msg("EOF detectd")
AppLogger.Trace().Msg("Sending EOF to the console displayer")
channels.MessageBuffer <- "\nEOF"
//sending the exit marker to the diplay goroutine
Comment on lines +126 to +129
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of exposing your synchronization primitive, how about hiding this being an interface, something like.

type MessageWriter interface {
       WriteMessage(msg string) error
}

And hide the "technicalities` of how you actually send a message behind this ?
It will give you much better readability and testabillity 😸

From the caller It will be much more easier:

_ = w.WriteMessage("\nEOF")

AppLogger.Trace().Msg("Sending exit marker to the console displayer")
channels.MessageBuffer <- exitString
//waiting for it to complete (blocking read)
AppLogger.Debug().Msg("Waiting for the signal that the display processing was completed")
<-channels.DisplayCompleted
AppLogger.Debug().Msg("Display processing completed (received signal)")
break
}

// TODO: move this out and use a channel for that
consoleDisplay.Add(string(buff[:n]))
channels.MessageBuffer <- string(buff[:n])

if closeRequested {
consoleDisplay.Add("\nExiting...\n")
//sending the exit marker to the diplay goroutine
AppLogger.Trace().Msg("Sending exit marker to the console displayer as we received the exit sequence")
channels.MessageBuffer <- exitString
//waiting for it to complete (blocking read)
AppLogger.Debug().Msg("Waiting for the signal that the display processing was completed")
<-channels.DisplayCompleted
AppLogger.Debug().Msg("Display processing completed (received signal)")
break
}
}
AppLogger.Debug().Msg("Sending signal that all processing is done")
channels.Done <- true

AppLogger.Debug().Msg("Exiting Listen")
return nil
}

// Tries to auto detect the Morserino port
// Tries to auto detect the Morserino port based on the USB Vendor and Device ID
func DetectDevice(genericEnumPorts comPortEnumerator) (string, error) {
theComPortList, err := Get_com_list(genericEnumPorts)
if err != nil {
Expand Down
Loading