APP-4940 - Update fake encoder config to have `ticks_per_sec` parameter #3976

zaporter-work · 2024-05-20T21:37:48Z

Useful for viz team config testing: https://viam.atlassian.net/browse/APP-4940

New proposed config for a fake encoder:

    {
      "name": "encoder-1",
      "namespace": "rdk",
      "type": "encoder",
      "model": "fake",
      "attributes": {
        "update_rate_msec": 100,
        "ticks_per_sec": 610
      }
    },

I also changed this to use a StoppableWorkers group because there was a subtle bug with the context being canceled too early

zaporter-work · 2024-05-20T21:39:07Z

components/encoder/fake/encoder.go

@@ -69,7 +71,8 @@ func (e *fakeEncoder) Reconfigure(

 // Config describes the configuration of a fake encoder.
 type Config struct {
-	UpdateRate int64 `json:"update_rate_msec,omitempty"`
+	UpdateRate int64   `json:"update_rate_msec,omitempty"`
+	Speed      float64 `json:"speed,omitempty"`


Should this have units? If so, what would they be?

I was thinking about:

Suggested change

Speed float64 `json:"speed,omitempty"`

Speed float64 `json:"speed_tpm,omitempty"`

where tpm means ticks per minute, but I'm not sure if that is clear? (or because it is fake, if speed is fine)

maybe just ticks_per_sec, since that kinda matches the guidelines

tpm wouldn't be obvious to me. I think because it's fake, speed is fine, in the way that, when you're playing a board game, the units on the currency are often just called "money" ("I will pay 3 money to buy that potion").

Oh, I like John's suggestion! ticks_per_sec or ticks_per_min is clearer than tps or tpm. My main problem is that ticks is such a rare unit that I couldn't tell what the t stood for.

Oh, I really like ticks_per_sec @JohnN193 . That makes sense to me. Will swap to that (might involve a few other code changes because the rest of this logic is measured in ticks per minute)

Updated in 773457b to ticks_per_sec.

I also changed Speed to ticksPerSec in a few other places

JohnN193 · 2024-05-20T21:49:07Z

components/encoder/fake/encoder.go

@@ -69,7 +71,8 @@ func (e *fakeEncoder) Reconfigure(

 // Config describes the configuration of a fake encoder.
 type Config struct {
-	UpdateRate int64 `json:"update_rate_msec,omitempty"`
+	UpdateRate int64   `json:"update_rate_msec,omitempty"`
+	Speed      float64 `json:"speed,omitempty"`


maybe just ticks_per_sec, since that kinda matches the guidelines

components/encoder/fake/encoder_test.go

components/encoder/fake/encoder.go

penguinland · 2024-05-20T21:52:53Z

components/encoder/fake/encoder.go

@@ -69,7 +71,8 @@ func (e *fakeEncoder) Reconfigure(

 // Config describes the configuration of a fake encoder.
 type Config struct {
-	UpdateRate int64 `json:"update_rate_msec,omitempty"`
+	UpdateRate int64   `json:"update_rate_msec,omitempty"`
+	Speed      float64 `json:"speed,omitempty"`


Oh, I like John's suggestion! ticks_per_sec or ticks_per_min is clearer than tps or tpm. My main problem is that ticks is such a rare unit that I couldn't tell what the t stood for.

penguinland · 2024-05-20T21:54:51Z

components/encoder/fake/encoder.go


-	e.start(ctx)


Dang, I think this just fixed a bug! ctx was an argument to this function, and would likely be cancelled by the end of reconfiguration. By switching to a StoppableWorkers, the context used in the start function won't be cancelled until someone calls Close on the component. Thanks!

heh, the two bugs canceled each other out: our background goroutine would shut down unexpectedly early, and then when we don't shut it down during Close, we don't leak goroutines. Thanks for fixing both these issues! 😂

Agreed -- When I saw these issues, StoppableWorkers is the first thing that came to mind. Thanks for making it!

randhid

I can't look at this today, so requesting changes until I can since I noticed John approved.

Typically attribute additions also go through a scope doc review.

penguinland · 2024-05-20T22:03:42Z

The failed test is an unrelated flaky one. Ping the Jira ticket in the hopes that someone looks at it soon, and retry the tests.

randhid · 2024-05-23T14:51:10Z

components/encoder/fake/encoder.go

+		}
+
+		e.mu.Lock()
+		e.position += int64(e.ticksPerSec / float64(1000/updateRate))


This thing can never go backwards, and would need a dependency on motor to figure out if it would have to. Needs a bit of thought.

this is the same behavior as speed from before though right? we can pass in a negative ticksPerSec as well. Additionally this is still a fake encoder, not intended to be used with a real motor iirc

They used to be. Right now, we can do this since we needed the fake encoder from gpio motor's encoded_motor_tests; it used to be that the tests called SetSpeed directly use the encoder in tests. I think if we're going to add functionality, we should think a bit more about this, maybe migrate the SetSpeed and SetPosition to DoCommands to be cleaner? That we it's more representative of an actual encoder.

randhid · 2024-05-23T14:52:44Z

components/encoder/fake/encoder.go

@@ -69,7 +71,8 @@ func (e *fakeEncoder) Reconfigure(

 // Config describes the configuration of a fake encoder.
 type Config struct {
-	UpdateRate int64 `json:"update_rate_msec,omitempty"`
+	UpdateRate  int64   `json:"update_rate_msec,omitempty"`
+	TicksPerSec float64 `json:"ticks_per_sec,omitempty"`


Instead of this - and dealing with the zero default value that will have the driver stall, let's do a non-user facing default speed value at the top of this file.

You can also use the update rate multiplied by a default constant if you prefer.

I don't think this pr should change the default behavior of fake encoders, that would require changing more of the tests.

I don't think the tests are testing with any encoder functionality from a cursory look, so it can just be changed in the fake motor and gpio motor tests to injects.

randhid · 2024-05-23T15:31:00Z

I can't look at this today, so requesting changes until I can since I noticed John approved.

Typically attribute additions also go through a scope doc review.

@zaporter-work, if you need a fake encoder, I can make and upload one to my fake modules module, as an alternative to more work on this one.

randhid

Okay, @JohnN193 convinced me - add a warning in Reconfigure to tell you that the encoder will do nothing if the speed is 0, and counts up if the speed is positive, counts down if the speed is negative.

zaporter-work · 2024-05-23T20:47:59Z

(rand and I talked a bit offline -- deleted speed. Plan to wait for at least tomorrow to let this sit and we can ponder if we like this idea)

penguinland · 2024-05-23T20:48:57Z

components/encoder/fake/encoder.go

+// Close safely shuts down the fake encoder.
+func (e *fakeEncoder) Close(ctx context.Context) error {
+	e.mu.Lock()
+	defer e.mu.Unlock()


No need to lock the mutex: it protects the position, updateRate, and similar things, but we're not using any of those. e.workers.Stop() is already atomic, protected by its own internal mutex.

components/motor/fake/motor_test.go

zaporter-work · 2024-05-24T17:48:42Z

After talking with my team a bit more, I think I've come to realize that the viz team goals might not be aligned with the goals of the fakes. I've realized that we also probably want fake encoder to support position in degrees and probably have 1-2 other config params.

As it is, I am opposed to merging this because I don't think it solves a concrete problem (because we still need more params) and it changes the behavior of the fake motor & leaves the user with no way to stop the encoder from ticking upwards.

I think it makes a lot more sense for us to independently develop some stub components in a module that fit our needs and then in a few months, we can decide if we (as a company) want to incorporate those changes back into the fakes.

Thank you everyone for your time. I really deeply appreciate it.

If anyone wants to take any of this code and use it in a future PR, please don't let me closing this discourage that. I am closing this partially because I am not confident that I am the right person to be making these changes.

initial

f371d35

viambot added the safe to test This pull request is marked safe to test from a trusted zone label May 20, 2024

zaporter-work commented May 20, 2024

View reviewed changes

zaporter-work requested review from penguinland, randhid, martha-johnston and JohnN193 May 20, 2024 21:39

JohnN193 approved these changes May 20, 2024

View reviewed changes

penguinland reviewed May 20, 2024

View reviewed changes

randhid requested changes May 20, 2024

View reviewed changes

speed -> ticks per second

773457b

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels May 20, 2024

zaporter-work changed the title ~~APP-4940 - Update fake encoder config to have speed parameter~~ APP-4940 - Update fake encoder config to have ticks_per_second parameter May 20, 2024

not TriviallyClosable

2820f0a

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels May 20, 2024

zaporter-work changed the title ~~APP-4940 - Update fake encoder config to have ticks_per_second parameter~~ APP-4940 - Update fake encoder config to have ticks_per_sec parameter May 21, 2024

martha-johnston approved these changes May 22, 2024

View reviewed changes

randhid requested changes May 23, 2024

View reviewed changes

randhid approved these changes May 23, 2024

View reviewed changes

add warning to reconfigure if ticks_per_sec is zero

dda31d8

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels May 23, 2024

update rate msec was truncating and did not work

ee2365e

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels May 23, 2024

no more speed

e42d697

delete all speed references

9dcd30b

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels May 23, 2024

lint

45efe95

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels May 23, 2024

penguinland reviewed May 23, 2024

View reviewed changes

components/motor/fake/motor_test.go Outdated Show resolved Hide resolved

revert motor_test changes

0216229

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels May 23, 2024

zaporter-work closed this May 24, 2024

zaporter-work deleted the zp/encoder-with-speed branch May 24, 2024 17:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

APP-4940 - Update fake encoder config to have `ticks_per_sec` parameter #3976

APP-4940 - Update fake encoder config to have `ticks_per_sec` parameter #3976

zaporter-work commented May 20, 2024 •

edited

zaporter-work May 20, 2024

JohnN193 May 20, 2024

penguinland May 20, 2024

penguinland May 20, 2024

zaporter-work May 20, 2024

zaporter-work May 20, 2024

JohnN193 May 20, 2024

penguinland May 20, 2024

penguinland May 20, 2024

penguinland May 20, 2024

zaporter-work May 20, 2024

randhid left a comment

penguinland commented May 20, 2024

randhid May 23, 2024

JohnN193 May 23, 2024

randhid May 23, 2024

randhid May 23, 2024

JohnN193 May 23, 2024

randhid May 23, 2024

randhid commented May 23, 2024 •

edited

randhid left a comment

zaporter-work commented May 23, 2024

penguinland May 23, 2024 •

edited

zaporter-work commented May 24, 2024

	Speed float64 `json:"speed,omitempty"`
	Speed float64 `json:"speed_tpm,omitempty"`

APP-4940 - Update fake encoder config to have ticks_per_sec parameter #3976

APP-4940 - Update fake encoder config to have ticks_per_sec parameter #3976

Conversation

zaporter-work commented May 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

randhid left a comment

Choose a reason for hiding this comment

penguinland commented May 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

randhid commented May 23, 2024 • edited

randhid left a comment

Choose a reason for hiding this comment

zaporter-work commented May 23, 2024

penguinland May 23, 2024 • edited

Choose a reason for hiding this comment

zaporter-work commented May 24, 2024

APP-4940 - Update fake encoder config to have `ticks_per_sec` parameter #3976

APP-4940 - Update fake encoder config to have `ticks_per_sec` parameter #3976

zaporter-work commented May 20, 2024 •

edited

randhid commented May 23, 2024 •

edited

penguinland May 23, 2024 •

edited