Skip to content

Acu add sin el nod#1072

Draft
mjrand wants to merge 3 commits into
mainfrom
ACU_add_sin_el_nod
Draft

Acu add sin el nod#1072
mjrand wants to merge 3 commits into
mainfrom
ACU_add_sin_el_nod

Conversation

@mjrand

@mjrand mjrand commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Added the gen_sin_el_nod scan generator function into the ACU drivers.

Description

Added a new generator function for generating sinusoidal elevation nods into the ACU agent. This generator will allow us to generate sine wave el nods that do not move in azimuth. This is basically a replica of type3 scan behavior with no azimuth movement.

Motivation and Context

LAT ISO review requested sinusoidal elevation nods with no azimuth movement.

How Has This Been Tested?

I've simmed the gen_sin_el_nod driver function output with success. The generator function should work as intended.
image

I have not yet tested the changes to the agent generate_scan function or tested an movement on the LAT. This needs to be tested before PR approval.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@mjrand mjrand requested a review from mhasself June 9, 2026 21:28
@mjrand mjrand self-assigned this Jun 9, 2026
@mhasself

Copy link
Copy Markdown
Member

Thanks Michael. I recommend that you move this to "draft" PR until we finalize design and test it on a real system.

Initial reactions:

  • Why should this be a new "scan type" for generate_scan rather than a new, stand-alone process / task? The arguments for this functionality are a tiny subset of the full az scan functionality. It's not like someone would decide, one day, to do an el-nod everywhere that they previously were doing a scan!
  • In modern application, num_batches and batch_size are deprecated. Let's not introduce new usages ... i.e., they can be removed from your driver function (and just hard-code the current default at the top of the function).
  • Is there a control on step_time relative to the el_freq? Make sure step_time gives you "enough" points. Fourier limit is 4 but I would think we'd want more like 16.
  • The generator sets group_flag=0, which means a "stop" can cause the track to terminate at ~any point. That might "work" but it would be best to tune group_flag such that stops will always occur at the bottom of the dip (which is also the starting point).
  • To support the preceding point, it might be helpful to tweak step_time such that it divides the oscillation period evenly. Then just generate 1 cycle at a time and yield it (another reason to drop the "batch_size" notion). Can discuss more elsewhere. But basically set new_step_time = (el_freq*round((el_freq*step_time)**-1))**-1.

@mjrand mjrand marked this pull request as draft June 10, 2026 21:08
@mjrand

mjrand commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Aaaah I didn't even know there was a draft PR feature... that's super helpful! (I am not great at git... lol). Changed.

  1. There's really no reason other than I wanted to reuse the generate_scan architecture and make it really easy to call from with sorunlib. It should be somewhat easy to make its own process/task. I'll write that in. I wasn't super attached to keeping it in generate_scan instead of making its own process.
  2. Yeah the notes on batches makes sense to me. I can rewrite this to always yield out whole number nods. That should also prevent us from stopping the elevation on a non-zero velocity.... which opens up a question of if we need to also write a smooth-stop generator for elevation (I think we probably do), but that's outside the scope of this PR.
  3. I think step_size = 0.1 is probably good enough for all nods that aren't hilariously high frequency. I think there is a check for this in plan_scan... but it should probably just be in this function calculated like you say.
  4. Agreed we should probably yield whole number nods.
  5. Agreed.

I think rewriting the driver to yield whole number of nods and then rewriting the agent process will be good. One of the worries I had with the current implementation into generate_scan was how plan_scan would interact with az_vel=0. I think it would work... but I think if we make a generate_el_nod function or whatever it could just start at the current az instead of having to run-up to az_vel=0. I'll try this implementation and see how it would work.

@mjrand

mjrand commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Alright I reverted generate_scan and made generate_sin_el_nod. I started it as a copy of generate_scan and then tried to prune back as many things weren't used or weren't necessary. This meant pruning plan_scan and some other misc variables like wait_to_start (which I couldn't find used in generate_scan either...). I also fixed up the generate_sin_el_nod drivers function to only yield full el nods and to always divide them into 16 points (which can be changed...). This means step_time is determined in the driver and doesn't need to be callable in the agent.

@mhasself mhasself left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks! More notes ...

Comment thread socs/agents/acu/agent.py
self._simple_process_stop,
blocking=False,
startup=False)
agent.register_process('generate_sin_el_nod',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's just call this "generate_elnod". If we invent alternatives, we'll likely jam them into this function.

Comment thread socs/agents/acu/agent.py
Comment on lines +2420 to +2422
@ocs_agent.param('az', type=float)
@ocs_agent.param('el_endpoint1', type=float, default=None)
@ocs_agent.param('el_endpoint2', type=float, default=None)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Instead of having these as args, let's just make this function run them at the current position. This makes for less bookkeeping in sorunlib. And then you'd just have "el_depth", which could be negative, as the parameter that determines how far apart the endpoints are.

Comment thread socs/agents/acu/agent.py
'el1': el_endpoint1,
'el2': el_endpoint2,
'el_freq': el_freq,
'type': 'sin_el_nod',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The data here need to be exactly the same (same fields with same data types) as the ones in generate_scan. Otherwise the feed will complain (or, at best, data loading of this info will be totally messed up). I think you should indeed use 4 for the sin_el_nod -- and register it in the main list of scan types. Any irrelevant stuff should be set to 0 or nan or something like that.

Comment thread socs/agents/acu/agent.py
if not self._get_sun_policy('map_valid'):
return False, 'Sun Safety Map not computed or stale; run the monitor_sun process.'

n = max(2, int(np.ceil((el2 - el1) / 1.)))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

humor me and throw an "abs" in front of that (el2 - el1) ...

MIN_STEP_TIME = 0.05 # in seconds.

# Get el throw
el_throw = abs(el_endpoint2 - el_endpoint1) / 2

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we want to allow downward nods; so remove the abs().

raise ValueError("El Freq is too high! El Freq must be < 1.25Hz to keep "
"step_time > 0.05!")

step_time = POINTS_PER_NOD * el_freq # Divide nod into 16 points

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ya I don't think we should force step_time like this. Rather, respect what the user asked for but:

  • round it, so it evenly divides the nod period.
  • make sure there are at least 16 points per period.

That's what I described in previous comment. We'll likely need to experiment with this a little to see what step_times work well.

timestamp=t + t0,
az=az, el=el, az_vel=0, el_vel=el_vel,
az_flag=0, el_flag=1,
group_flag=0))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

set group_flag=1 except for the final point in each cycle -- that is what will actually force only complete cycles to be uploaded.

az_flag=0, el_flag=1,
group_flag=0))

t += step_time

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Consider restructuring the loop like this:

# Templates for one cycle
template_t = step_time * np.arange(points_per_nod)
template_el, template_el_vel = get_el(template_t)  # vectors

while not done():
   point_block = [PointBlock(timestamp=t0 + t, ..., group_flag=1)
                       for (t, el, el_v) in zip(template_t, template_el, template_el_vel)]
   point_block[0].group_flag = 0  # Ok to stop on the zero vel starting point!
   yield point_block
   t0 += 1 / el_freq

# And one last zero-vel point.
yield [PointBlock(timestamp=t0, ... vels=0, group_flag=0)]

This reduces floating point errors, but also just makes it crystal clear that we're doing one cycle at a time, and they're always identical in el shape.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants